From eb08c3fdb9f64508dc9e6c6c0cc0285a5884b43b Mon Sep 17 00:00:00 2001
From: jmoore <josh@glencoesoftware.com>
Date: Thu, 20 Sep 2018 13:03:31 +0200
Subject: [PATCH 01/55] Add S3Handle using minio 5.0.0

URLs starting with `s3://` will now be passed to the
S3Handle. `./showinfo s3://bucket/path/file.tiff` then
opens the file as expected.
---
 pom.xml                                 |   5 +
 src/main/java/loci/common/Location.java |   5 +-
 src/main/java/loci/common/S3Handle.java | 117 ++++++++++++++++++++++++
 3 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100644 src/main/java/loci/common/S3Handle.java

diff --git a/pom.xml b/pom.xml
index 58d6478a..4c9d61b7 100644
--- a/pom.xml
+++ b/pom.xml
@@ -101,6 +101,11 @@
   </distributionManagement>
 
   <dependencies>
+    <dependency>
+     <groupId>io.minio</groupId>
+     <artifactId>minio</artifactId>
+     <version>5.0.0</version>
+    </dependency>
     <dependency>
      <groupId>com.esotericsoftware.kryo</groupId>
      <artifactId>kryo</artifactId>
diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 585eb853..508b7f06 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -395,7 +395,10 @@ public static IRandomAccess getHandle(String id, boolean writable,
       LOGGER.trace("no handle was mapped for this ID");
       String mapId = getMappedId(id);
 
-      if (id.startsWith("http://") || id.startsWith("https://")) {
+      if (id.startsWith("s3://")) {
+        handle = new S3Handle(mapId);
+      }
+      else if (id.startsWith("http://") || id.startsWith("https://")) {
         handle = new URLHandle(mapId);
       }
       else if (allowArchiveHandles && ZipHandle.isZipFile(mapId)) {
diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
new file mode 100644
index 00000000..0b740a98
--- /dev/null
+++ b/src/main/java/loci/common/S3Handle.java
@@ -0,0 +1,117 @@
+/*
+ * #%L
+ * Common package for I/O and related utilities
+ * %%
+ * Copyright (C) 2018 Open Microscopy Environment:
+ *   - Board of Regents of the University of Wisconsin-Madison
+ *   - Glencoe Software, Inc.
+ *   - University of Dundee
+ * %%
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ * #L%
+ */
+
+package loci.common;
+
+import java.io.BufferedInputStream;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+
+import io.minio.MinioClient;
+import io.minio.ObjectStat;
+import io.minio.errors.*;
+import org.xmlpull.v1.XmlPullParserException;
+
+/**
+ * Provides random access to S3 buckets using the IRandomAccess interface.
+ * Instances of S3Handle are read-only.
+ *
+ * @see IRandomAccess
+ * @see StreamHandle
+ * @see java.net.URLConnection
+ *
+ */
+public class S3Handle extends StreamHandle {
+
+  private final static String DEFAULT_SERVER = "https://s3.amazonaws.com";
+
+  private String server;
+
+  private String url;
+
+  private String bucket;
+
+  private String path;
+
+  private MinioClient s3Client;
+
+  public S3Handle(String url) throws IOException {
+    this(DEFAULT_SERVER, url);
+  }
+
+
+  public S3Handle(String server, String url) throws IOException {
+    if (!url.startsWith("s3") && !url.startsWith("file:")) {
+      url = "s3://" + url;
+    }
+    this.server = server;
+    this.url = url;
+    try {
+      URI parser = new URI(url);
+      bucket = parser.getAuthority();
+      path = parser.getPath();
+    } catch (URISyntaxException e) {
+      throw new RuntimeException(e);
+    }
+    resetStream();
+  }
+
+  @Override
+  protected void resetStream() throws IOException {
+    try {
+      s3Client = new MinioClient(server);
+      ObjectStat stat = s3Client.statObject(bucket, path);
+      length = stat.length();
+      stream = new DataInputStream(new BufferedInputStream(
+              s3Client.getObject(bucket, path)));
+      stream.skip(-1L);
+      fp = 0;
+      mark = 0;
+    } catch (InvalidEndpointException |
+            InvalidPortException |
+            InvalidBucketNameException |
+            NoSuchAlgorithmException |
+            InsufficientDataException |
+            InvalidKeyException |
+            NoResponseException |
+            XmlPullParserException |
+            ErrorResponseException |
+            InternalException |
+            InvalidArgumentException e) {
+      throw new IOException("failed to load s3: " + url, e);
+    }
+  }
+}

From 3289c57e7e5650bc7dbc168490c20b9b3b35d7e8 Mon Sep 17 00:00:00 2001
From: jmoore <josh@glencoesoftware.com>
Date: Thu, 27 Sep 2018 00:37:17 +0200
Subject: [PATCH 02/55] Location: wider s3 support including parent/child logic

Changes include:
 - url may now be null even if isURL is not
 - Location(parent, child) is now the primary constructor
 - several special cases for s3 at the moment

Remaining TODOs:
 - untangle isDirectory/isFile/exists calls
 - fix list()
 - handle http:// similarly to s3://
 - add caching for various internal calls like exists()
 - remove openConnection wherever possible
 - handle absolute paths (cc: Seb)
---
 src/main/java/loci/common/Location.java       | 105 ++++++++++++------
 .../java/loci/common/utests/LocationTest.java |  21 ++--
 2 files changed, 86 insertions(+), 40 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 508b7f06..e9aeaaf9 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -37,9 +37,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLConnection;
+import java.net.*;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -93,8 +91,9 @@ protected class ListingsResult {
 
   // -- Fields --
 
-  private boolean isURL = true;
+  private boolean isURL = false;
   private URL url;
+  private URI uri;
   private File file;
 
   // -- Constructors --
@@ -107,22 +106,7 @@ protected class ListingsResult {
    * @see #getMappedFile(String)
    */
   public Location(String pathname) {
-    LOGGER.trace("Location({})", pathname);
-    if (pathname.contains("://")) {
-      // Avoid expensive exception handling in case when path is
-      // obviously not an URL
-      try {
-        url = new URL(getMappedId(pathname));
-      }
-      catch (MalformedURLException e) {
-        LOGGER.trace("Location is not a URL", e);
-        isURL = false;
-      }
-    } else {
-      LOGGER.trace("Location is not a URL");
-      isURL = false;
-    }
-    if (!isURL) file = new File(getMappedId(pathname));
+    this((String) null, pathname);
   }
 
   /**
@@ -145,7 +129,40 @@ public Location(File file) {
    * @param child the relative path name
    */
   public Location(String parent, String child) {
-    this(parent + File.separator + child);
+    LOGGER.trace("Location({}, {})", parent, child);
+
+    String mapped = null;
+    String pathname = null;
+
+    // First handle possible URIs
+    if (child.contains("://")) {
+      // Avoid expensive exception handling in case when path is
+      // obviously not an URL
+      try {
+        mapped = getMappedId(child);
+        pathname = child;
+        uri = new URI(mapped);
+        isURL = true;
+        url = uri.toURL();
+      }
+      catch (URISyntaxException | MalformedURLException e) {
+        // TODO: this should possibly throw
+        // possibly leaves url null
+      }
+    }
+
+    // If not a URI, then deal with relative vs. absolute paths
+    if (pathname == null) {
+      if (parent != null) {
+        pathname = parent + File.separator + child;
+      } else {
+        pathname = child;
+      }
+      mapped = getMappedId(pathname);
+    }
+
+    if (!isURL) file = new File(mapped);
+
   }
 
   /**
@@ -467,6 +484,17 @@ public String[] list(boolean noHiddenFiles) {
     final List<String> files = new ArrayList<String>();
     if (isURL) {
       try {
+        if (url == null) {
+          // Likely s3
+          if (!exists()) {
+            return null;
+          }
+          if (uri.toString().endsWith("/")) {
+            return new String[0];
+          } else {
+            return null;
+          }
+        }
         URLConnection c = url.openConnection();
         InputStream is = c.getInputStream();
         boolean foundEnd = false;
@@ -542,7 +570,8 @@ public String[] list(boolean noHiddenFiles) {
    */
   public boolean canRead() {
     LOGGER.trace("canRead()");
-    return isURL ? (isDirectory() || isFile() || exists()) : file.canRead();
+    // Note: isFile calls exist
+    return isURL ? (isDirectory() || isFile()) : file.canRead();
   }
 
   /**
@@ -645,7 +674,9 @@ public boolean exists() {
     LOGGER.trace("exists()");
     if (isURL) {
       try {
-        url.getContent();
+        // TODO: existence should almost certainly be cached.
+        IRandomAccess handle = getHandle(uri.toString());
+        handle.length();
         return true;
       }
       catch (IOException e) {
@@ -678,7 +709,7 @@ public Location getAbsoluteFile() {
    */
   public String getAbsolutePath() {
     LOGGER.trace("getAbsolutePath()");
-    return isURL ? url.toExternalForm() : file.getAbsolutePath();
+    return isURL ? uri.normalize().toString() : file.getAbsolutePath();
   }
 
   /**
@@ -718,9 +749,8 @@ public String getCanonicalPath() throws IOException {
   public String getName() {
     LOGGER.trace("getName()");
     if (isURL) {
-      String name = url.getFile();
-      name = name.substring(name.lastIndexOf("/") + 1);
-      return name;
+      // TODO: we should just store new File(uri) in file
+      return  new File(uri.getPath()).getName();
     }
     return file.getName();
   }
@@ -760,7 +790,7 @@ public Location getParentFile() {
    * @see java.io.File#getPath()
    */
   public String getPath() {
-    return isURL ? url.getHost() + url.getPath() : file.getPath();
+    return isURL ? uri.getHost() + uri.getPath() : file.getPath();
   }
 
   /**
@@ -772,7 +802,7 @@ public String getPath() {
    */
   public boolean isAbsolute() {
     LOGGER.trace("isAbsolute()");
-    return isURL ? true : file.isAbsolute();
+    return isURL ? uri.isAbsolute() : file.isAbsolute();
   }
 
   /**
@@ -784,8 +814,13 @@ public boolean isAbsolute() {
   public boolean isDirectory() {
     LOGGER.trace("isDirectory()");
     if (isURL) {
-      String[] list = list();
-      return list != null;
+      if ("s3".equals(uri.getScheme())) {
+        return uri.toString().endsWith("/") && exists();
+      } else {
+        // TODO: this should be removed as well.
+        String[] list = list();
+        return list != null;
+      }
     }
     return file.isDirectory();
   }
@@ -832,7 +867,11 @@ public long lastModified() {
     LOGGER.trace("lastModified()");
     if (isURL) {
       try {
-        return url.openConnection().getLastModified();
+        if (url != null) {
+          return url.openConnection().getLastModified();
+        } else {
+          return 0;
+        }
       }
       catch (IOException e) {
         LOGGER.trace("Could not determine URL's last modification time", e);
@@ -911,7 +950,7 @@ public URL toURL() throws MalformedURLException {
    */
   @Override
   public String toString() {
-    return isURL ? url.toString() : file.toString();
+    return isURL ? uri.toString() : file.toString();
   }
 
 }
diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index ddd47e78..f4a69e81 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -36,6 +36,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.net.MalformedURLException;
 import java.net.Socket;
 import java.net.URL;
 import java.util.Arrays;
@@ -92,27 +93,29 @@ public void setup() throws IOException {
       new Location("http://www.openmicroscopy.org/"),
       new Location("https://www.openmicroscopy.org/"),
       new Location("https://www.openmicroscopy.org/nonexisting"),
-      new Location(hiddenFile)
+      new Location(hiddenFile),
+      new Location("s3://server/bucket-dne/key/"),
+      new Location("s3://server/bucket-dne/key/file.tif"),
     };
 
     exists = new boolean[] {
-      true, false, true, true, true, false, true
+      true, false, true, true, true, false, true, false, false
     };
 
     isDirectory = new boolean[] {
-      false, false, true, false, false, false, false
+      false, false, true, false, false, false, false, false, false
     };
 
     isHidden = new boolean[] {
-      false, false, false, false, false, false, true
+      false, false, false, false, false, false, true, false, false
     };
 
     mode = new String[] {
-      "rw", "", "rw", "r", "r", "","rw"
+      "rw", "", "rw", "r", "r", "","rw", "", "" // S3 isn't readable
     };
 
     isRemote = new boolean[] {
-      false, false, false, true, true, true, false
+      false, false, false, true, true, true, false, true, true
     };
   }
 
@@ -244,7 +247,11 @@ public void testToURL() throws IOException {
       if (file.isDirectory() && !path.endsWith(File.separator)) {
         path += File.separator;
       }
-      assertEquals(file.getName(), file.toURL(), new URL(path));
+      try {
+        assertEquals(file.getName(), file.toURL(), new URL(path));
+      } catch (MalformedURLException e) {
+        assertEquals(path, true, path.contains("s3://"));
+      }
     }
   }
 

From 384e3972117229d2fc9e2f11b4f52bbe01cbdad1 Mon Sep 17 00:00:00 2001
From: jmoore <josh@glencoesoftware.com>
Date: Thu, 27 Sep 2018 00:41:07 +0200
Subject: [PATCH 03/55] S3Handle: parse more complex URLs

In order to allow specifying non-AWS buckets,
endpoints and similar information need to be
encoded in the URL.
---
 src/main/java/loci/common/S3Handle.java       | 120 +++++++++++++++---
 .../java/loci/common/utests/S3HandleTest.java | 106 ++++++++++++++++
 2 files changed, 205 insertions(+), 21 deletions(-)
 create mode 100644 src/test/java/loci/common/utests/S3HandleTest.java

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 0b740a98..16f2f339 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -35,10 +35,13 @@
 import java.io.BufferedInputStream;
 import java.io.DataInputStream;
 import java.io.IOException;
+import java.net.ConnectException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import io.minio.MinioClient;
 import io.minio.ObjectStat;
@@ -49,6 +52,8 @@
  * Provides random access to S3 buckets using the IRandomAccess interface.
  * Instances of S3Handle are read-only.
  *
+ * TODO: How does one handle buckets with periods
+ *
  * @see IRandomAccess
  * @see StreamHandle
  * @see java.net.URLConnection
@@ -56,43 +61,110 @@
  */
 public class S3Handle extends StreamHandle {
 
-  private final static String DEFAULT_SERVER = "https://s3.amazonaws.com";
+  public final static String DEFAULT_SERVER = "https://s3.amazonaws.com";
+
+  /** Format: "s3://accessKey:secrectKey@bucket.endpoint/key" */
+  public final static String URI_PATTERN = "(s3://)?" +
+          "((?<access>.*):(?<secret>.*)@)?" +
+          "(?<bucket>.*?)"+
+          "([.](?<server>.*?)((:)(?<port>\\d+))?)?"+
+          "/(?<path>.*)";
+
+  public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);
+
+  /** full string used to configure this handle */
+  private final String uri;
 
-  private String server;
+  /** access key, if provided */
+  private final String accessKey;
 
-  private String url;
+  /** secret key, if provided */
+  private final String secretKey;
 
-  private String bucket;
+  /** name of the bucket */
+  private final String bucket;
 
-  private String path;
+  /** endpoint to which requests will be sent */
+  private final String server;
+
+  /** port at the given server */
+  private final int port;
+
+  /** remaining path, or key, for this accessed resource */
+  private final String path;
 
   private MinioClient s3Client;
 
   public S3Handle(String url) throws IOException {
-    this(DEFAULT_SERVER, url);
+    this(null, url);
   }
 
+  public S3Handle(String server, String uri) throws IOException {
+    this(server, uri, true);
+  }
 
-  public S3Handle(String server, String url) throws IOException {
-    if (!url.startsWith("s3") && !url.startsWith("file:")) {
-      url = "s3://" + url;
+  public S3Handle(String uri, boolean initialize) throws IOException {
+    this(null, uri, initialize);
+  }
+
+  public S3Handle(String server, String uri, boolean initialize) throws IOException {
+    this.uri = uri;
+    Matcher m = URI_PARSER.matcher(uri);
+    if (!m.matches()) {
+      throw new RuntimeException(String.format(
+              "%s does not match pattern %s", uri, URI_PATTERN));
     }
-    this.server = server;
-    this.url = url;
-    try {
-      URI parser = new URI(url);
-      bucket = parser.getAuthority();
-      path = parser.getPath();
-    } catch (URISyntaxException e) {
-      throw new RuntimeException(e);
+    this.accessKey = m.group("access");
+    this.secretKey = m.group("secret");
+    this.bucket = m.group("bucket");
+    this.server = server(m, server);
+    this.path = m.group("path");
+    this.port = port(m);
+    if (initialize) {
+      resetStream();
+    }
+  }
+
+  private int port(Matcher m) {
+    String p = m.group("port");
+    if (p == null) {
+      return 0;
+    } else {
+      return Integer.valueOf(p);
     }
-    resetStream();
+  }
+
+  private String server(Matcher m, String server) {
+    String s = m.group("server");
+    if (s == null) {
+      s = server != null ? server : DEFAULT_SERVER;
+    }
+    if (!s.contains("://")) {
+      s = "http://" + s;
+    }
+    return s;
+  }
+
+  public String getServer() {
+    return server;
+  }
+
+  public int getPort() {
+    return port;
+  }
+
+  public String getBucket() {
+    return bucket;
+  }
+
+  public String getPath() {
+    return path;
   }
 
   @Override
   protected void resetStream() throws IOException {
     try {
-      s3Client = new MinioClient(server);
+      s3Client = new MinioClient(server, port, accessKey, secretKey);
       ObjectStat stat = s3Client.statObject(bucket, path);
       length = stat.length();
       stream = new DataInputStream(new BufferedInputStream(
@@ -100,7 +172,8 @@ protected void resetStream() throws IOException {
       stream.skip(-1L);
       fp = 0;
       mark = 0;
-    } catch (InvalidEndpointException |
+    } catch (ConnectException |
+            InvalidEndpointException |
             InvalidPortException |
             InvalidBucketNameException |
             NoSuchAlgorithmException |
@@ -111,7 +184,12 @@ protected void resetStream() throws IOException {
             ErrorResponseException |
             InternalException |
             InvalidArgumentException e) {
-      throw new IOException("failed to load s3: " + url, e);
+      throw new IOException(String.format(
+              "failed to load s3: %s\n" +
+              "\tserver:%s\n"+
+              "\tport:%d\n"+
+              "\tbucket:%s\n"+
+              "\tpath:%s", uri, server, port, bucket, path), e);
     }
   }
 }
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
new file mode 100644
index 00000000..b0c6acdf
--- /dev/null
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -0,0 +1,106 @@
+/*
+ * #%L
+ * Common package for I/O and related utilities
+ * %%
+ * Copyright (C) 2005 - 2016 Open Microscopy Environment:
+ *   - Board of Regents of the University of Wisconsin-Madison
+ *   - Glencoe Software, Inc.
+ *   - University of Dundee
+ * %%
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * 
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ * 
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ * #L%
+ */
+
+package loci.common.utests;
+
+import loci.common.S3Handle;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.testng.AssertJUnit.assertEquals;
+
+/**
+ * Unit tests for the loci.common.S3Handle class.
+ *
+ * @see loci.common.URLHandle
+ */
+public class S3HandleTest {
+
+  // -- Fields --
+
+  // -- Setup methods --
+
+  @BeforeMethod
+  public void setup() {
+    // no-op
+  }
+
+  // -- Test methods --
+
+  @Test
+  public void testParseDefault() throws IOException {
+    S3Handle s3 = new S3Handle("s3://bucket/key/file.tif", false);
+    assertEquals(S3Handle.DEFAULT_SERVER, s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
+
+  @Test
+  public void testParseLocalhost() throws IOException {
+    S3Handle s3 = new S3Handle("s3://bucket.localhost:9000/key/file.tif", false);
+    assertEquals("http://localhost", s3.getServer());
+    assertEquals(9000, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
+
+  @Test
+  public void testSetLocalhost() throws IOException {
+    S3Handle s3 = new S3Handle("localhost", "s3://bucket/key/file.tif", false);
+    assertEquals("http://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
+
+  @Test
+  public void testParseAuth() throws IOException {
+    S3Handle s3 = new S3Handle("s3://access:secret@bucket/key/file.tif", false);
+    assertEquals(S3Handle.DEFAULT_SERVER, s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
+
+  @Test
+  public void testParseAuthLocalhost() throws IOException {
+    S3Handle s3 = new S3Handle("s3://access:secret@bucket.localhost:9000/key/file.tif", false);
+    assertEquals("http://localhost", s3.getServer());
+    assertEquals(9000, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
+
+}

From f464dc3b73677323bc326f10279f1226b27fcb05 Mon Sep 17 00:00:00 2001
From: jmoore <josh@glencoesoftware.com>
Date: Fri, 28 Sep 2018 10:58:19 +0200
Subject: [PATCH 04/55] Drop JDK7 for the moment

---
 .travis.yml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index a3bf927d..1cc9c60c 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -10,7 +10,6 @@ cache:
 jdk:
   - oraclejdk11
   - oraclejdk8
-  - openjdk7
 
 matrix:
   fast_finish: true

From bfc6e8857724814d98e871e9d25cb8366b63157c Mon Sep 17 00:00:00 2001
From: jmoore <josh@glencoesoftware.com>
Date: Mon, 1 Oct 2018 09:46:42 +0200
Subject: [PATCH 05/55] Location: partially deal with null child arguments

---
 src/main/java/loci/common/Location.java | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index e9aeaaf9..30eb22c0 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -135,7 +135,7 @@ public Location(String parent, String child) {
     String pathname = null;
 
     // First handle possible URIs
-    if (child.contains("://")) {
+    if (child != null && child.contains("://")) {
       // Avoid expensive exception handling in case when path is
       // obviously not an URL
       try {
@@ -154,6 +154,7 @@ public Location(String parent, String child) {
     // If not a URI, then deal with relative vs. absolute paths
     if (pathname == null) {
       if (parent != null) {
+        // TODO: in some cases child here may be null
         pathname = parent + File.separator + child;
       } else {
         pathname = child;

From 38166c0abf57e92fbc75cf0a9aca1ca79aa18f48 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 4 Oct 2018 09:52:09 +0100
Subject: [PATCH 06/55] S3 bucket must be the first path component

bucket.with.dots.is.indistinguishable.from.server.endpoint
---
 src/main/java/loci/common/S3Handle.java | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 16f2f339..830f35f0 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -52,8 +52,6 @@
  * Provides random access to S3 buckets using the IRandomAccess interface.
  * Instances of S3Handle are read-only.
  *
- * TODO: How does one handle buckets with periods
- *
  * @see IRandomAccess
  * @see StreamHandle
  * @see java.net.URLConnection
@@ -63,11 +61,11 @@ public class S3Handle extends StreamHandle {
 
   public final static String DEFAULT_SERVER = "https://s3.amazonaws.com";
 
-  /** Format: "s3://accessKey:secrectKey@bucket.endpoint/key" */
+  /** Format: "s3://accessKey:secretKey@server-endpoint/bucket/path" */
   public final static String URI_PATTERN = "(s3://)?" +
           "((?<access>.*):(?<secret>.*)@)?" +
-          "(?<bucket>.*?)"+
-          "([.](?<server>.*?)((:)(?<port>\\d+))?)?"+
+          "((?<server>.*?)((:)(?<port>\\d+))?)?"+
+          "/(?<bucket>.*?)"+
           "/(?<path>.*)";
 
   public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);

From edb69c9f9f818712f0a03f00ba85d8d2dfcbd2a4 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 4 Oct 2018 16:34:14 +0100
Subject: [PATCH 07/55] Location.length return handle.length

---
 src/main/java/loci/common/Location.java | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 30eb22c0..4b63b11a 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -437,6 +437,7 @@ else if (allowArchiveHandles && BZip2Handle.isBZip2File(mapId)) {
           handle = new NIOFileHandle(mapId, writable ? "rw" : "r");
         }
       }
+      LOGGER.trace("Created new handle {} -> {}", id, handle);
     }
     LOGGER.trace("Location.getHandle: {} -> {}", id, handle);
     return handle;
@@ -678,6 +679,7 @@ public boolean exists() {
         // TODO: existence should almost certainly be cached.
         IRandomAccess handle = getHandle(uri.toString());
         handle.length();
+        handle.close();
         return true;
       }
       catch (IOException e) {
@@ -893,7 +895,10 @@ public long length() {
     LOGGER.trace("length()");
     if (isURL) {
       try {
-        return url.openConnection().getContentLength();
+        IRandomAccess handle = getHandle(uri.toString());
+        long len = handle.length();
+        handle.close();
+        return len;
       }
       catch (IOException e) {
         LOGGER.trace("Could not determine URL's content length", e);

From 0f4abc525600c9096c314139d78b3062868a43cb Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 11:09:41 +0100
Subject: [PATCH 08/55] S3 server and protocol are always required

Reduces the amount of magic in S3Handle

Also enables S3HandleTest
---
 src/main/java/loci/common/S3Handle.java       | 49 +++++++++++--------
 .../java/loci/common/utests/S3HandleTest.java | 37 ++++++--------
 2 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 830f35f0..cffe912c 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -62,9 +62,10 @@ public class S3Handle extends StreamHandle {
   public final static String DEFAULT_SERVER = "https://s3.amazonaws.com";
 
   /** Format: "s3://accessKey:secretKey@server-endpoint/bucket/path" */
-  public final static String URI_PATTERN = "(s3://)?" +
+  public final static String URI_PATTERN =
+          "(?<protocol>.+?)://" +
           "((?<access>.*):(?<secret>.*)@)?" +
-          "((?<server>.*?)((:)(?<port>\\d+))?)?"+
+          "(?<server>.+?)((:)(?<port>\\d+))?"+
           "/(?<bucket>.*?)"+
           "/(?<path>.*)";
 
@@ -93,19 +94,25 @@ public class S3Handle extends StreamHandle {
 
   private MinioClient s3Client;
 
+  /**
+   * Open an S3 file
+   *
+   * @param url the full URL to the S3 resource
+   * @throws IOException if there is an error during opening
+   */
   public S3Handle(String url) throws IOException {
-    this(null, url);
-  }
-
-  public S3Handle(String server, String uri) throws IOException {
-    this(server, uri, true);
+    this(url, true);
   }
 
+  /**
+   * Open an S3 file
+   *
+   * @param url the full URL to the S3 resource
+   * @param initialize If true open the stream, otherwise just parse connection
+   *        string
+   * @throws IOException if there is an error during opening
+   */
   public S3Handle(String uri, boolean initialize) throws IOException {
-    this(null, uri, initialize);
-  }
-
-  public S3Handle(String server, String uri, boolean initialize) throws IOException {
     this.uri = uri;
     Matcher m = URI_PARSER.matcher(uri);
     if (!m.matches()) {
@@ -115,7 +122,7 @@ public S3Handle(String server, String uri, boolean initialize) throws IOExceptio
     this.accessKey = m.group("access");
     this.secretKey = m.group("secret");
     this.bucket = m.group("bucket");
-    this.server = server(m, server);
+    this.server = server(m);
     this.path = m.group("path");
     this.port = port(m);
     if (initialize) {
@@ -132,15 +139,17 @@ private int port(Matcher m) {
     }
   }
 
-  private String server(Matcher m, String server) {
-    String s = m.group("server");
-    if (s == null) {
-      s = server != null ? server : DEFAULT_SERVER;
-    }
-    if (!s.contains("://")) {
-      s = "http://" + s;
+  private String server(Matcher m) {
+    String protocol = m.group("protocol");
+    if (protocol.equals("s3")) {
+      // TODO: Decide how to handle S3Handle reader settings
+      protocol = System.getenv("BF_S3_PROTOCOL");
+      if (protocol == null) {
+        // TODO: Default to https for improved security
+        protocol = "http";
+      }
     }
-    return s;
+    return protocol + "://" + m.group("server");
   }
 
   public String getServer() {
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index b0c6acdf..effc36bd 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -45,6 +45,7 @@
  *
  * @see loci.common.URLHandle
  */
+@Test(groups="readTests")
 public class S3HandleTest {
 
   // -- Fields --
@@ -58,37 +59,20 @@ public void setup() {
 
   // -- Test methods --
 
-  @Test
-  public void testParseDefault() throws IOException {
-    S3Handle s3 = new S3Handle("s3://bucket/key/file.tif", false);
-    assertEquals(S3Handle.DEFAULT_SERVER, s3.getServer());
-    assertEquals(0, s3.getPort());
-    assertEquals("bucket", s3.getBucket());
-    assertEquals("key/file.tif", s3.getPath());
-  }
-
   @Test
   public void testParseLocalhost() throws IOException {
-    S3Handle s3 = new S3Handle("s3://bucket.localhost:9000/key/file.tif", false);
+    S3Handle s3 = new S3Handle("s3://localhost:9000/bucket/key/file.tif", false);
     assertEquals("http://localhost", s3.getServer());
     assertEquals(9000, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
   }
 
-  @Test
-  public void testSetLocalhost() throws IOException {
-    S3Handle s3 = new S3Handle("localhost", "s3://bucket/key/file.tif", false);
-    assertEquals("http://localhost", s3.getServer());
-    assertEquals(0, s3.getPort());
-    assertEquals("bucket", s3.getBucket());
-    assertEquals("key/file.tif", s3.getPath());
-  }
-
   @Test
   public void testParseAuth() throws IOException {
-    S3Handle s3 = new S3Handle("s3://access:secret@bucket/key/file.tif", false);
-    assertEquals(S3Handle.DEFAULT_SERVER, s3.getServer());
+    S3Handle s3 = new S3Handle(
+      "s3://access:secret@s3.example.org/bucket/key/file.tif", false);
+    assertEquals("s3.example.org", s3.getServer());
     assertEquals(0, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
@@ -96,11 +80,20 @@ public void testParseAuth() throws IOException {
 
   @Test
   public void testParseAuthLocalhost() throws IOException {
-    S3Handle s3 = new S3Handle("s3://access:secret@bucket.localhost:9000/key/file.tif", false);
+    S3Handle s3 = new S3Handle("s3://access:secret@localhost:9000/bucket/key/file.tif", false);
     assertEquals("http://localhost", s3.getServer());
     assertEquals(9000, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
   }
 
+  @Test
+  public void testParseProtocol() throws IOException {
+    S3Handle s3 = new S3Handle(
+      "example://localhost/bucket/key/file.tif", false);
+    assertEquals("example://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
 }

From 91bcbe1ee0b9e300f116d88687a6e5c9292dc2f6 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 11:14:18 +0100
Subject: [PATCH 09/55] Add in-depth traces to S3Handle

---
 src/main/java/loci/common/S3Handle.java     |  6 +++++-
 src/main/java/loci/common/StreamHandle.java | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index cffe912c..87261c72 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -48,6 +48,9 @@
 import io.minio.errors.*;
 import org.xmlpull.v1.XmlPullParserException;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Provides random access to S3 buckets using the IRandomAccess interface.
  * Instances of S3Handle are read-only.
@@ -59,7 +62,7 @@
  */
 public class S3Handle extends StreamHandle {
 
-  public final static String DEFAULT_SERVER = "https://s3.amazonaws.com";
+  private static final Logger LOGGER = LoggerFactory.getLogger(S3Handle.class);
 
   /** Format: "s3://accessKey:secretKey@server-endpoint/bucket/path" */
   public final static String URI_PATTERN =
@@ -170,6 +173,7 @@ public String getPath() {
 
   @Override
   protected void resetStream() throws IOException {
+    LOGGER.trace("Resetting");
     try {
       s3Client = new MinioClient(server, port, accessKey, secretKey);
       ObjectStat stat = s3Client.statObject(bucket, path);
diff --git a/src/main/java/loci/common/StreamHandle.java b/src/main/java/loci/common/StreamHandle.java
index 8a088c85..9853046e 100644
--- a/src/main/java/loci/common/StreamHandle.java
+++ b/src/main/java/loci/common/StreamHandle.java
@@ -39,6 +39,9 @@
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Abstract IRandomAccess implementation for reading from InputStreams and
  * writing to OutputStreams.
@@ -49,6 +52,8 @@
  */
 public abstract class StreamHandle implements IRandomAccess {
 
+  private static final Logger LOGGER = LoggerFactory.getLogger(StreamHandle.class);
+
   // -- Fields --
 
   /** Name of the open stream. */
@@ -89,6 +94,7 @@ public StreamHandle() {
   /* @see IRandomAccess#close() */
   @Override
   public void close() throws IOException {
+    LOGGER.trace("closing");
     length = fp = mark = 0;
     if (stream != null) stream.close();
     if (outStream != null) outStream.close();
@@ -106,18 +112,22 @@ public long getFilePointer() throws IOException {
   /* @see IRandomAccess#length() */
   @Override
   public long length() throws IOException {
+    // Too verbose
+    // LOGGER.trace("{}", length);
     return length;
   }
 
   /* @see IRandomAccess#read(byte[]) */
   @Override
   public int read(byte[] b) throws IOException {
+    LOGGER.trace("0 {}", b.length);
     return read(b, 0, b.length);
   }
 
   /* @see IRandomAccess#read(byte[], int, int) */
   @Override
   public int read(byte[] b, int off, int len) throws IOException {
+    LOGGER.trace("{} {}", off, len);
     int n = stream.read(b, off, len);
     if (n >= 0) fp += n;
     else n = 0;
@@ -139,6 +149,7 @@ public int read(ByteBuffer buffer) throws IOException {
   /* @see IRandomAccess#read(ByteBuffer, int, int) */
   @Override
   public int read(ByteBuffer buffer, int off, int len) throws IOException {
+    LOGGER.trace("{} {}", off, len);
     if (buffer.hasArray()) {
       return read(buffer.array(), off, len);
     }
@@ -152,6 +163,7 @@ public int read(ByteBuffer buffer, int off, int len) throws IOException {
   /* @see IRandomAccess#seek(long) */
   @Override
   public void seek(long pos) throws IOException {
+    LOGGER.trace("{}", pos);
     long diff = pos - fp;
     fp = pos;
 
@@ -170,12 +182,14 @@ public void seek(long pos) throws IOException {
   /* @see IRandomAccess.write(ByteBuffer) */
   @Override
   public void write(ByteBuffer buf) throws IOException {
+    LOGGER.trace("0 {}", buf.capacity());
     write(buf, 0, buf.capacity());
   }
 
   /* @see IRandomAccess.write(ByteBuffer, int, int) */
   @Override
   public void write(ByteBuffer buf, int off, int len) throws IOException {
+    LOGGER.trace("{} {}", off, len);
     buf.position(off);
     if (buf.hasArray()) {
       write(buf.array(), off, len);

From 028e0a0e99db4427ec7de86b3142b30cc5f9a2c9 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 11:29:04 +0100
Subject: [PATCH 10/55] Make default s3 protocol a var

---
 src/main/java/loci/common/S3Handle.java | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 87261c72..342ca651 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -62,6 +62,11 @@
  */
 public class S3Handle extends StreamHandle {
 
+  /** Default protocol for fetching s3://
+   # TODO: Default to https for improved security
+   */
+  public final static String DEFAULT_S3_PROTOCOL = "http";
+
   private static final Logger LOGGER = LoggerFactory.getLogger(S3Handle.class);
 
   /** Format: "s3://accessKey:secretKey@server-endpoint/bucket/path" */
@@ -148,8 +153,7 @@ private String server(Matcher m) {
       // TODO: Decide how to handle S3Handle reader settings
       protocol = System.getenv("BF_S3_PROTOCOL");
       if (protocol == null) {
-        // TODO: Default to https for improved security
-        protocol = "http";
+        protocol = DEFAULT_S3_PROTOCOL;
       }
     }
     return protocol + "://" + m.group("server");

From abcec314c52d2bf6907ebc3df3ef649421bdd58e Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 11:29:48 +0100
Subject: [PATCH 11/55] Fix S3HandleTest protocol

---
 src/test/java/loci/common/utests/S3HandleTest.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index effc36bd..d9d97fa0 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -72,7 +72,7 @@ public void testParseLocalhost() throws IOException {
   public void testParseAuth() throws IOException {
     S3Handle s3 = new S3Handle(
       "s3://access:secret@s3.example.org/bucket/key/file.tif", false);
-    assertEquals("s3.example.org", s3.getServer());
+    assertEquals("http://s3.example.org", s3.getServer());
     assertEquals(0, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());

From dd61bb81156082e693874d2011715badf699e354 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 14:01:15 +0100
Subject: [PATCH 12/55] S3Handle default to https, add mock object for testing

---
 src/main/java/loci/common/S3Handle.java       | 19 ++++++++---
 src/main/java/loci/common/StreamHandle.java   |  7 ++++
 .../java/loci/common/utests/S3HandleTest.java | 34 +++++++++++++++----
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 342ca651..092172d2 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -65,7 +65,7 @@ public class S3Handle extends StreamHandle {
   /** Default protocol for fetching s3://
    # TODO: Default to https for improved security
    */
-  public final static String DEFAULT_S3_PROTOCOL = "http";
+  public final static String DEFAULT_S3_PROTOCOL = "https";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3Handle.class);
 
@@ -79,6 +79,9 @@ public class S3Handle extends StreamHandle {
 
   public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);
 
+  /** S3 configuration */
+  private final Settings settings;
+
   /** full string used to configure this handle */
   private final String uri;
 
@@ -109,7 +112,7 @@ public class S3Handle extends StreamHandle {
    * @throws IOException if there is an error during opening
    */
   public S3Handle(String url) throws IOException {
-    this(url, true);
+    this(url, true, null);
   }
 
   /**
@@ -118,9 +121,16 @@ public S3Handle(String url) throws IOException {
    * @param url the full URL to the S3 resource
    * @param initialize If true open the stream, otherwise just parse connection
    *        string
+   * @param s custom settings object
    * @throws IOException if there is an error during opening
    */
-  public S3Handle(String uri, boolean initialize) throws IOException {
+  public S3Handle(String uri, boolean initialize, Settings s) throws IOException {
+    if (s == null) {
+      this.settings = new StreamHandle.Settings();
+    }
+    else {
+      this.settings = s;
+    }
     this.uri = uri;
     Matcher m = URI_PARSER.matcher(uri);
     if (!m.matches()) {
@@ -150,8 +160,7 @@ private int port(Matcher m) {
   private String server(Matcher m) {
     String protocol = m.group("protocol");
     if (protocol.equals("s3")) {
-      // TODO: Decide how to handle S3Handle reader settings
-      protocol = System.getenv("BF_S3_PROTOCOL");
+      protocol = this.settings.get("BF_S3_PROTOCOL");
       if (protocol == null) {
         protocol = DEFAULT_S3_PROTOCOL;
       }
diff --git a/src/main/java/loci/common/StreamHandle.java b/src/main/java/loci/common/StreamHandle.java
index 9853046e..bca93d23 100644
--- a/src/main/java/loci/common/StreamHandle.java
+++ b/src/main/java/loci/common/StreamHandle.java
@@ -52,6 +52,13 @@
  */
 public abstract class StreamHandle implements IRandomAccess {
 
+  // TODO: Decide how to handle S3Handle and other reader settings
+  public static class Settings {
+    public String get(String key) {
+      return System.getenv(key);
+    }
+  }
+
   private static final Logger LOGGER = LoggerFactory.getLogger(StreamHandle.class);
 
   // -- Fields --
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index d9d97fa0..4a0636fe 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -33,6 +33,7 @@
 package loci.common.utests;
 
 import loci.common.S3Handle;
+import loci.common.StreamHandle;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
@@ -61,8 +62,8 @@ public void setup() {
 
   @Test
   public void testParseLocalhost() throws IOException {
-    S3Handle s3 = new S3Handle("s3://localhost:9000/bucket/key/file.tif", false);
-    assertEquals("http://localhost", s3.getServer());
+    S3Handle s3 = new S3Handle("s3://localhost:9000/bucket/key/file.tif", false, null);
+    assertEquals("https://localhost", s3.getServer());
     assertEquals(9000, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
@@ -71,8 +72,8 @@ public void testParseLocalhost() throws IOException {
   @Test
   public void testParseAuth() throws IOException {
     S3Handle s3 = new S3Handle(
-      "s3://access:secret@s3.example.org/bucket/key/file.tif", false);
-    assertEquals("http://s3.example.org", s3.getServer());
+      "s3://access:secret@s3.example.org/bucket/key/file.tif", false, null);
+    assertEquals("https://s3.example.org", s3.getServer());
     assertEquals(0, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
@@ -80,8 +81,8 @@ public void testParseAuth() throws IOException {
 
   @Test
   public void testParseAuthLocalhost() throws IOException {
-    S3Handle s3 = new S3Handle("s3://access:secret@localhost:9000/bucket/key/file.tif", false);
-    assertEquals("http://localhost", s3.getServer());
+    S3Handle s3 = new S3Handle("s3://access:secret@localhost:9000/bucket/key/file.tif", false, null);
+    assertEquals("https://localhost", s3.getServer());
     assertEquals(9000, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
@@ -90,10 +91,29 @@ public void testParseAuthLocalhost() throws IOException {
   @Test
   public void testParseProtocol() throws IOException {
     S3Handle s3 = new S3Handle(
-      "example://localhost/bucket/key/file.tif", false);
+      "example://localhost/bucket/key/file.tif", false, null);
     assertEquals("example://localhost", s3.getServer());
     assertEquals(0, s3.getPort());
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
   }
+
+  private class MockSettings extends StreamHandle.Settings {
+    public String get(String key) {
+      if (key.equals("BF_S3_PROTOCOL")) {
+        return "custom";
+      }
+      return super.get(key);
+    }
+  }
+
+  @Test
+  public void testDefaultProtocol() throws IOException {
+    StreamHandle.Settings s = new MockSettings();
+    S3Handle s3 = new S3Handle("s3://localhost/bucket/key/file.tif", false, s);
+    assertEquals("custom://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals("key/file.tif", s3.getPath());
+  }
 }

From a7479b4aad72a3fafd11711771331ab4fa98a1b8 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 14:13:34 +0100
Subject: [PATCH 13/55] Add S3Handle.toString()

---
 src/main/java/loci/common/S3Handle.java | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 092172d2..eb1e236e 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -209,11 +209,12 @@ protected void resetStream() throws IOException {
             InternalException |
             InvalidArgumentException e) {
       throw new IOException(String.format(
-              "failed to load s3: %s\n" +
-              "\tserver:%s\n"+
-              "\tport:%d\n"+
-              "\tbucket:%s\n"+
-              "\tpath:%s", uri, server, port, bucket, path), e);
+              "failed to load s3: %s\n\t%s", uri, this), e);
     }
   }
+
+  public String toString() {
+    return String.format("server:%s port:%d bucket:%s path:%s",
+                          server, port, bucket, path);
+  }
 }

From 817e88d54638f61c4a99f11ccfab49229c66a41f Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 14:47:54 +0100
Subject: [PATCH 14/55] Add caching to S3Handle

---
 src/main/java/loci/common/S3Handle.java     | 66 ++++++++++++++++++++-
 src/main/java/loci/common/StreamHandle.java |  4 ++
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index eb1e236e..d82d7b6a 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -38,12 +38,16 @@
 import java.net.ConnectException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import io.minio.MinioClient;
+import io.minio.errors.MinioException;
 import io.minio.ObjectStat;
 import io.minio.errors.*;
 import org.xmlpull.v1.XmlPullParserException;
@@ -74,8 +78,8 @@ public class S3Handle extends StreamHandle {
           "(?<protocol>.+?)://" +
           "((?<access>.*):(?<secret>.*)@)?" +
           "(?<server>.+?)((:)(?<port>\\d+))?"+
-          "/(?<bucket>.*?)"+
-          "/(?<path>.*)";
+          "(/(?<bucket>.+?))?"+
+          "(/(?<path>.+))?";
 
   public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);
 
@@ -184,6 +188,64 @@ public String getPath() {
     return path;
   }
 
+  /**
+   * Download an S3 object to a file system cache if it doesn't already exist
+   *
+   * @param url the full URL to the S3 resource
+   * @param s custom settings object
+   * @return File path to the cached object
+   * @throws IOException if there is an error during reading or writing
+   */
+  public static String cacheObject(String url, Settings s) throws
+      IOException,
+      HandleException {
+    S3Handle s3 = new S3Handle(url, false, s);
+    String cacheroot = s.getRemoteCacheRootDir();
+    if (cacheroot == null) {
+      throw new HandleException("Remote cache root dir is not set");
+    }
+    // TODO: Need to ensure this path is safe. Is there a Java method to check?
+    String cacheobj = s3.getCacheKey();
+    // Hopefully creates a cross-platform path
+    Path cachepath = Paths.get(cacheroot, cacheobj);
+
+    if (Files.exists(cachepath)) {
+      LOGGER.debug("Found existing cache for {} at {}", s3, cachepath);
+    }
+    else {
+      LOGGER.debug("Caching {} to {}", s3, cachepath);
+      Files.createDirectories(cachepath.getParent());
+      s3.downloadObject(cachepath.toString());
+      LOGGER.debug("Downloaded {}", cachepath);
+    }
+    return cachepath.toString();
+  }
+
+  public String getCacheKey(){
+    String cachekey =
+      getServer().replace("://", "/") + "/" +
+      getPort() + "/" +
+      getBucket() + "/" +
+      getPath();
+    return cachekey;
+  }
+
+  protected void downloadObject(String destination) throws HandleException {
+    LOGGER.trace("destination:{}", destination);
+    try {
+      s3Client = new MinioClient(server, port, accessKey, secretKey);
+      ObjectStat stat = s3Client.statObject(bucket, path);
+      s3Client.getObject(bucket, path, destination);
+    } catch (
+      IOException |
+      InvalidKeyException |
+      MinioException |
+      NoSuchAlgorithmException |
+      XmlPullParserException e) {
+        throw new HandleException("Download failed " + toString(), e);
+      }
+  }
+
   @Override
   protected void resetStream() throws IOException {
     LOGGER.trace("Resetting");
diff --git a/src/main/java/loci/common/StreamHandle.java b/src/main/java/loci/common/StreamHandle.java
index bca93d23..2200092a 100644
--- a/src/main/java/loci/common/StreamHandle.java
+++ b/src/main/java/loci/common/StreamHandle.java
@@ -57,6 +57,10 @@ public static class Settings {
     public String get(String key) {
       return System.getenv(key);
     }
+
+    public String getRemoteCacheRootDir() {
+      return get("BF_REMOTE_CACHE_ROOTDIR");
+    }
   }
 
   private static final Logger LOGGER = LoggerFactory.getLogger(StreamHandle.class);

From edd1d94a475fc1a1e766464d6efdb8af0d5ad24d Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 14:48:51 +0100
Subject: [PATCH 15/55] Cache S3 files when BF_REMOTE_CACHE_ROOTDIR set

---
 src/main/java/loci/common/Location.java | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 4b63b11a..ad18a607 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -414,7 +414,20 @@ public static IRandomAccess getHandle(String id, boolean writable,
       String mapId = getMappedId(id);
 
       if (id.startsWith("s3://")) {
-        handle = new S3Handle(mapId);
+        StreamHandle.Settings ss = new StreamHandle.Settings();
+        if (ss.getRemoteCacheRootDir() != null) {
+          String cachedFile = S3Handle.cacheObject(mapId, ss);
+          if (bufferSize > 0) {
+            handle = new NIOFileHandle(
+              new File(cachedFile), "r", bufferSize);
+          }
+          else {
+            handle = new NIOFileHandle(cachedFile, "r");
+          }
+        }
+        else {
+          handle = new S3Handle(mapId);
+        }
       }
       else if (id.startsWith("http://") || id.startsWith("https://")) {
         handle = new URLHandle(mapId);

From ef322944198a4234bcf11b0ca9337d0089bf6de4 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 5 Oct 2018 15:12:31 +0100
Subject: [PATCH 16/55] S3Handle: only create cache dir if s3 object exists

---
 src/main/java/loci/common/S3Handle.java | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index d82d7b6a..ef70733e 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -214,8 +214,7 @@ public static String cacheObject(String url, Settings s) throws
     }
     else {
       LOGGER.debug("Caching {} to {}", s3, cachepath);
-      Files.createDirectories(cachepath.getParent());
-      s3.downloadObject(cachepath.toString());
+      s3.downloadObject(cachepath);
       LOGGER.debug("Downloaded {}", cachepath);
     }
     return cachepath.toString();
@@ -230,12 +229,13 @@ public String getCacheKey(){
     return cachekey;
   }
 
-  protected void downloadObject(String destination) throws HandleException {
+  protected void downloadObject(Path destination) throws HandleException {
     LOGGER.trace("destination:{}", destination);
     try {
       s3Client = new MinioClient(server, port, accessKey, secretKey);
       ObjectStat stat = s3Client.statObject(bucket, path);
-      s3Client.getObject(bucket, path, destination);
+      Files.createDirectories(destination.getParent());
+      s3Client.getObject(bucket, path, destination.toString());
     } catch (
       IOException |
       InvalidKeyException |

From c23ed879981938d7e09d39cf7c1798842549b57f Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Mon, 8 Oct 2018 18:19:27 +0100
Subject: [PATCH 17/55] Throw HandleException if downloading null path

---
 src/main/java/loci/common/S3Handle.java | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index ef70733e..4bf44300 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -231,6 +231,9 @@ public String getCacheKey(){
 
   protected void downloadObject(Path destination) throws HandleException {
     LOGGER.trace("destination:{}", destination);
+    if (path == null) {
+      throw new HandleException("Download path=null not allowed");
+    }
     try {
       s3Client = new MinioClient(server, port, accessKey, secretKey);
       ObjectStat stat = s3Client.statObject(bucket, path);

From 8920edb6cac12b5d16ddb962e8a2d47abd128506 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 10 Oct 2018 10:27:18 +0100
Subject: [PATCH 18/55] Custom protocol is handled as s3+protocol:// instead of
 env-var

---
 src/main/java/loci/common/S3Handle.java            |  8 ++++----
 src/test/java/loci/common/utests/S3HandleTest.java | 12 +-----------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 4bf44300..97145d67 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -164,10 +164,10 @@ private int port(Matcher m) {
   private String server(Matcher m) {
     String protocol = m.group("protocol");
     if (protocol.equals("s3")) {
-      protocol = this.settings.get("BF_S3_PROTOCOL");
-      if (protocol == null) {
-        protocol = DEFAULT_S3_PROTOCOL;
-      }
+      protocol = DEFAULT_S3_PROTOCOL;
+    }
+    else if (protocol.startsWith("s3+")) {
+      protocol = protocol.substring(3);
     }
     return protocol + "://" + m.group("server");
   }
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index 4a0636fe..1236e62b 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -98,19 +98,9 @@ public void testParseProtocol() throws IOException {
     assertEquals("key/file.tif", s3.getPath());
   }
 
-  private class MockSettings extends StreamHandle.Settings {
-    public String get(String key) {
-      if (key.equals("BF_S3_PROTOCOL")) {
-        return "custom";
-      }
-      return super.get(key);
-    }
-  }
-
   @Test
   public void testDefaultProtocol() throws IOException {
-    StreamHandle.Settings s = new MockSettings();
-    S3Handle s3 = new S3Handle("s3://localhost/bucket/key/file.tif", false, s);
+    S3Handle s3 = new S3Handle("s3+custom://localhost/bucket/key/file.tif", false, null);
     assertEquals("custom://localhost", s3.getServer());
     assertEquals(0, s3.getPort());
     assertEquals("bucket", s3.getBucket());

From 4756a08aee43542fd8143a22cc6f23d6d3fef9d4 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 10 Oct 2018 15:26:03 +0100
Subject: [PATCH 19/55] More testing/fixes to ensure consistent s3 url parsing

---
 src/main/java/loci/common/S3Handle.java       |  4 +--
 .../java/loci/common/utests/S3HandleTest.java | 36 +++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 97145d67..28990aef 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -78,8 +78,8 @@ public class S3Handle extends StreamHandle {
           "(?<protocol>.+?)://" +
           "((?<access>.*):(?<secret>.*)@)?" +
           "(?<server>.+?)((:)(?<port>\\d+))?"+
-          "(/(?<bucket>.+?))?"+
-          "(/(?<path>.+))?";
+          "(/(?<bucket>.+?)?)?"+
+          "(/(?<path>.+)?)?";
 
   public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);
 
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index 1236e62b..a3f26e6b 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -106,4 +106,40 @@ public void testDefaultProtocol() throws IOException {
     assertEquals("bucket", s3.getBucket());
     assertEquals("key/file.tif", s3.getPath());
   }
+
+  @Test
+  public void testParseNoSlash() throws IOException {
+    S3Handle s3 = new S3Handle("s3://localhost", false, null);
+    assertEquals("https://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals(null, s3.getBucket());
+    assertEquals(null, s3.getPath());
+  }
+
+  @Test
+  public void testParseSlashNoBucket() throws IOException {
+    S3Handle s3 = new S3Handle("s3://localhost/", false, null);
+    assertEquals("https://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals(null, s3.getBucket());
+    assertEquals(null, s3.getPath());
+  }
+
+  @Test
+  public void testParseBucketNoSlash() throws IOException {
+    S3Handle s3 = new S3Handle("s3://localhost/bucket", false, null);
+    assertEquals("https://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals(null, s3.getPath());
+  }
+
+  @Test
+  public void testParseBucketSlash() throws IOException {
+    S3Handle s3 = new S3Handle("s3://localhost/bucket/", false, null);
+    assertEquals("https://localhost", s3.getServer());
+    assertEquals(0, s3.getPort());
+    assertEquals("bucket", s3.getBucket());
+    assertEquals(null, s3.getPath());
+  }
 }

From bc0271140ece8cd6f7c6a1aaa4e954863ee3fe3e Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 10 Oct 2018 16:09:37 +0100
Subject: [PATCH 20/55] S3Handle determines whether it can handle URLs

---
 src/main/java/loci/common/Location.java            |  2 +-
 src/main/java/loci/common/S3Handle.java            |  9 +++++++++
 src/test/java/loci/common/utests/S3HandleTest.java | 10 ++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index ad18a607..51777aa9 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -413,7 +413,7 @@ public static IRandomAccess getHandle(String id, boolean writable,
       LOGGER.trace("no handle was mapped for this ID");
       String mapId = getMappedId(id);
 
-      if (id.startsWith("s3://")) {
+      if (S3Handle.canHandleScheme(id)) {
         StreamHandle.Settings ss = new StreamHandle.Settings();
         if (ss.getRemoteCacheRootDir() != null) {
           String cachedFile = S3Handle.cacheObject(mapId, ss);
diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 28990aef..ed43bd93 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -83,6 +83,8 @@ public class S3Handle extends StreamHandle {
 
   public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);
 
+  protected final static Pattern SCHEME_PARSER = Pattern.compile("s3(\\+\\p{Alnum}+)?://.*");
+
   /** S3 configuration */
   private final Settings settings;
 
@@ -109,6 +111,13 @@ public class S3Handle extends StreamHandle {
 
   private MinioClient s3Client;
 
+  /**
+   * Return true if this is a URL with an s3 scheme
+   */
+  public static boolean canHandleScheme(String url) {
+    return SCHEME_PARSER.matcher(url).matches();
+  }
+
   /**
    * Open an S3 file
    *
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index a3f26e6b..318e7bb8 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -40,6 +40,8 @@
 import java.io.IOException;
 
 import static org.testng.AssertJUnit.assertEquals;
+import static org.testng.AssertJUnit.assertFalse;
+import static org.testng.AssertJUnit.assertTrue;
 
 /**
  * Unit tests for the loci.common.S3Handle class.
@@ -60,6 +62,14 @@ public void setup() {
 
   // -- Test methods --
 
+  @Test
+  public void canHandleScheme() {
+    assertTrue(S3Handle.canHandleScheme("s3://"));
+    assertTrue(S3Handle.canHandleScheme("s3+transport://abc"));
+    assertFalse(S3Handle.canHandleScheme("s345://"));
+    assertFalse(S3Handle.canHandleScheme("http+s3://"));
+  }
+
   @Test
   public void testParseLocalhost() throws IOException {
     S3Handle s3 = new S3Handle("s3://localhost:9000/bucket/key/file.tif", false, null);

From 97bef004e03815f0eb0b92f872aa5fd57392adc4 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 10 Oct 2018 16:46:41 +0100
Subject: [PATCH 21/55] Use java.net.URI for parsing s3 URL

---
 src/main/java/loci/common/S3Handle.java | 107 ++++++++++++++----------
 1 file changed, 63 insertions(+), 44 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index ed43bd93..6d42ecda 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -66,30 +66,18 @@
  */
 public class S3Handle extends StreamHandle {
 
-  /** Default protocol for fetching s3://
-   # TODO: Default to https for improved security
-   */
+  /** Default protocol for fetching s3:// */
   public final static String DEFAULT_S3_PROTOCOL = "https";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3Handle.class);
 
-  /** Format: "s3://accessKey:secretKey@server-endpoint/bucket/path" */
-  public final static String URI_PATTERN =
-          "(?<protocol>.+?)://" +
-          "((?<access>.*):(?<secret>.*)@)?" +
-          "(?<server>.+?)((:)(?<port>\\d+))?"+
-          "(/(?<bucket>.+?)?)?"+
-          "(/(?<path>.+)?)?";
-
-  public final static Pattern URI_PARSER = Pattern.compile(URI_PATTERN);
-
   protected final static Pattern SCHEME_PARSER = Pattern.compile("s3(\\+\\p{Alnum}+)?://.*");
 
   /** S3 configuration */
   private final Settings settings;
 
-  /** full string used to configure this handle */
-  private final String uri;
+  /** Parsed URI used to configure this handle */
+  private final URI uri;
 
   /** access key, if provided */
   private final String accessKey;
@@ -137,48 +125,79 @@ public S3Handle(String url) throws IOException {
    * @param s custom settings object
    * @throws IOException if there is an error during opening
    */
-  public S3Handle(String uri, boolean initialize, Settings s) throws IOException {
+  public S3Handle(String uristr, boolean initialize, Settings s) throws
+      IOException {
     if (s == null) {
       this.settings = new StreamHandle.Settings();
     }
     else {
       this.settings = s;
     }
-    this.uri = uri;
-    Matcher m = URI_PARSER.matcher(uri);
-    if (!m.matches()) {
-      throw new RuntimeException(String.format(
-              "%s does not match pattern %s", uri, URI_PATTERN));
-    }
-    this.accessKey = m.group("access");
-    this.secretKey = m.group("secret");
-    this.bucket = m.group("bucket");
-    this.server = server(m);
-    this.path = m.group("path");
-    this.port = port(m);
-    if (initialize) {
-      resetStream();
+
+    try {
+      this.uri = new URI(uristr);
+    } catch (URISyntaxException e) {
+      throw new RuntimeException("Invalid URI " + uristr, e);
     }
-  }
 
-  private int port(Matcher m) {
-    String p = m.group("port");
-    if (p == null) {
-      return 0;
-    } else {
-      return Integer.valueOf(p);
+    // access[:secret]
+    String auth = this.uri.getUserInfo();
+    String accessKey = null;
+    String secretKey = null;
+    if (auth != null) {
+      String[] authparts = auth.split(":", 2);
+      accessKey = authparts[0];
+      if (authparts.length > 1) {
+        secretKey = authparts[1];
+      }
     }
-  }
+    this.accessKey = accessKey;
+    this.secretKey = secretKey;
 
-  private String server(Matcher m) {
-    String protocol = m.group("protocol");
-    if (protocol.equals("s3")) {
+    String protocol;
+    String scheme = this.uri.getScheme();
+    if (scheme.equals("s3")) {
       protocol = DEFAULT_S3_PROTOCOL;
     }
-    else if (protocol.startsWith("s3+")) {
-      protocol = protocol.substring(3);
+    else if (scheme.startsWith("s3+")) {
+      protocol = scheme.substring(3);
+    }
+    else {
+      protocol = scheme;
+    }
+    this.server = protocol + "://" + this.uri.getHost();
+
+    if (this.uri.getPort() == -1) {
+      this.port = 0;
+    }
+    else {
+      this.port = this.uri.getPort();
+    }
+
+    // First path component is the bucket
+    // TODO: Parsing this seems way more complicated than it should be
+    String fullpath = this.uri.getPath();
+    if (fullpath == null || fullpath.length() == 0) {
+      fullpath = "/";
+    }
+    // Leading / means first element is always ""
+    String[] pathparts = fullpath.split("/", 3);
+    if (pathparts[1].length() > 0) {
+      this.bucket = pathparts[1];
+    }
+    else {
+      this.bucket = null;
+    }
+    if (pathparts.length > 2 && pathparts[2].length() > 0) {
+      this.path = pathparts[2];
+    }
+    else {
+      this.path = null;
+    }
+
+    if (initialize) {
+      resetStream();
     }
-    return protocol + "://" + m.group("server");
   }
 
   public String getServer() {

From 4a6bdfe03a902217a9b74d89f201baf3b772679e Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 11 Oct 2018 15:06:07 +0100
Subject: [PATCH 22/55] S3 isDirectory equivalent to bucket-exists

S3 doesn't have directories, only key names which permit "/" so you can't easily check for the existance of a "directory".
---
 src/main/java/loci/common/Location.java       | 37 ++++++++++++++-----
 src/main/java/loci/common/S3Handle.java       | 31 ++++++++++++++++
 .../java/loci/common/utests/LocationTest.java |  8 ++--
 3 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 51777aa9..4353b92d 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -501,12 +501,14 @@ public String[] list(boolean noHiddenFiles) {
       try {
         if (url == null) {
           // Likely s3
-          if (!exists()) {
-            return null;
-          }
-          if (uri.toString().endsWith("/")) {
+          if (S3Handle.canHandleScheme(uri.getScheme()) && isDirectory()) {
+            // TODO: This is complicated, not sure what to do here
+            // See comment in isDirectory()
+            LOGGER.trace("list s3 {}: Returning []", uri);
             return new String[0];
-          } else {
+          }
+          else {
+            LOGGER.trace("list s3 {}: Returning null", uri);
             return null;
           }
         }
@@ -583,7 +585,7 @@ public String[] list(boolean noHiddenFiles) {
    * @return see above
    * @see java.io.File#canRead()
    */
-  public boolean canRead() {
+  public boolean canRead() throws IOException {
     LOGGER.trace("canRead()");
     // Note: isFile calls exist
     return isURL ? (isDirectory() || isFile()) : file.canRead();
@@ -827,11 +829,26 @@ public boolean isAbsolute() {
    * @return true if this pathname exists and represents a directory.
    * @see java.io.File#isDirectory()
    */
-  public boolean isDirectory() {
+  public boolean isDirectory() throws IOException {
     LOGGER.trace("isDirectory()");
     if (isURL) {
-      if ("s3".equals(uri.getScheme())) {
-        return uri.toString().endsWith("/") && exists();
+      if (S3Handle.canHandleScheme(uri.getScheme())) {
+        // TODO: This is complicated
+        //
+        // S3 doesn't have directories, but keys can contain / which we
+        // can pretend is a file path. However this "directory" doesn't
+        // actually exist, only the "contents" of the directory exist.
+        //
+        // Minio.listObjects() lists all objects in a bucket that
+        // match an optional prefix so this could be an option for checking
+        // whether to trest this as a directory.
+        //
+        // S3 buckets are the closest thing to a proper directory
+        // so for now
+        S3Handle h = new S3Handle(uri.toString(), false, null);
+        boolean isBucket = h.isBucket();
+        h.close();
+        return isBucket;
       } else {
         // TODO: this should be removed as well.
         String[] list = list();
@@ -847,7 +864,7 @@ public boolean isDirectory() {
    * @return true if this pathname exists and represents a regular file.
    * @see java.io.File#exists()
    */
-  public boolean isFile() {
+  public boolean isFile() throws IOException {
     LOGGER.trace("isFile()");
     return isURL ? (!isDirectory() && exists()) : file.isFile();
   }
diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 6d42ecda..8ccfcad7 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -277,9 +277,40 @@ protected void downloadObject(Path destination) throws HandleException {
       }
   }
 
+  /**
+   * Is this an accessible bucket?
+   *
+   * @return True if a bucket
+   * @throws IOException if there is an error during reading or writing
+   */
+  public boolean isBucket() throws IOException {
+    if (bucket == null || path !=null ) {
+      return false;
+    }
+    try {
+      s3Client = new MinioClient(server, port, accessKey, secretKey);
+      boolean isBucket = s3Client.bucketExists(bucket);
+      LOGGER.debug("isBucket? {} {}", this, isBucket);
+      return isBucket;
+    } catch (
+      InvalidKeyException |
+      MinioException |
+      NoSuchAlgorithmException |
+      XmlPullParserException e) {
+      throw new IOException(String.format(
+              "bucketExists failed: %s", this), e);
+    }
+  }
+
   @Override
   protected void resetStream() throws IOException {
     LOGGER.trace("Resetting");
+    if (bucket == null) {
+      throw new IOException("bucket is null");
+    }
+    if (path == null) {
+      throw new IOException("path is null");
+    }
     try {
       s3Client = new MinioClient(server, port, accessKey, secretKey);
       ObjectStat stat = s3Client.statObject(bucket, path);
diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index f4a69e81..72efb0d1 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -138,7 +138,7 @@ private void skipIfOffline(int i) throws SkipException {
   // -- Tests --
 
   @Test
-  public void testReadWriteMode() {
+  public void testReadWriteMode() throws IOException {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
       String msg = files[i].getName();
@@ -180,14 +180,14 @@ public void testParent() {
   }
 
   @Test
-  public void testIsDirectory() {
+  public void testIsDirectory() throws IOException {
     for (int i=0; i<files.length; i++) {
       assertEquals(files[i].getName(), files[i].isDirectory(), isDirectory[i]);
     }
   }
 
   @Test
-  public void testIsFile() {
+  public void testIsFile() throws IOException {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
       assertEquals(files[i].getName(), files[i].isFile(),
@@ -203,7 +203,7 @@ public void testIsHidden() {
   }
 
   @Test
-  public void testListFiles() {
+  public void testListFiles() throws IOException {
     for (int i=0; i<files.length; i++) {
       String[] completeList = files[i].list();
       String[] unhiddenList = files[i].list(true);

From bba2fa103c1cb32b75274dee66b36ed67640f227 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 11 Oct 2018 15:07:55 +0100
Subject: [PATCH 23/55] S3Handle.canHandleScheme can test scheme only

---
 src/main/java/loci/common/S3Handle.java            | 2 +-
 src/test/java/loci/common/utests/S3HandleTest.java | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 8ccfcad7..48ccb1cb 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -71,7 +71,7 @@ public class S3Handle extends StreamHandle {
 
   private static final Logger LOGGER = LoggerFactory.getLogger(S3Handle.class);
 
-  protected final static Pattern SCHEME_PARSER = Pattern.compile("s3(\\+\\p{Alnum}+)?://.*");
+  protected final static Pattern SCHEME_PARSER = Pattern.compile("s3(\\+\\p{Alnum}+)?(://.*)?");
 
   /** S3 configuration */
   private final Settings settings;
diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index 318e7bb8..405b7763 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -63,11 +63,13 @@ public void setup() {
   // -- Test methods --
 
   @Test
-  public void canHandleScheme() {
+  public void testCanHandleScheme() {
     assertTrue(S3Handle.canHandleScheme("s3://"));
     assertTrue(S3Handle.canHandleScheme("s3+transport://abc"));
+    assertTrue(S3Handle.canHandleScheme("s3+transport"));
     assertFalse(S3Handle.canHandleScheme("s345://"));
     assertFalse(S3Handle.canHandleScheme("http+s3://"));
+    assertFalse(S3Handle.canHandleScheme("https"));
   }
 
   @Test

From c102bb915727e71cd99f48ab9dfcad0f4d900d38 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 12 Oct 2018 16:44:48 +0100
Subject: [PATCH 24/55] Be explicit about S3 instead of inferring from
 isUrl/null

---
 src/main/java/loci/common/Location.java | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 4353b92d..3f8932c9 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -62,6 +62,12 @@ public class Location {
   private static final boolean IS_WINDOWS =
     System.getProperty("os.name").startsWith("Windows");
 
+  // -- Enumerations --
+  protected enum UrlType {
+    GENERIC,
+    S3
+  };
+
   // -- Static fields --
 
   /** Map from given filenames to actual filenames. */
@@ -92,6 +98,7 @@ protected class ListingsResult {
   // -- Fields --
 
   private boolean isURL = false;
+  private UrlType urlType;
   private URL url;
   private URI uri;
   private File file;
@@ -143,6 +150,12 @@ public Location(String parent, String child) {
         pathname = child;
         uri = new URI(mapped);
         isURL = true;
+        if (S3Handle.canHandleScheme(uri.toString())) {
+          urlType = UrlType.S3;
+        }
+        else {
+          urlType = UrlType.GENERIC;
+        }
         url = uri.toURL();
       }
       catch (URISyntaxException | MalformedURLException e) {
@@ -499,9 +512,8 @@ public String[] list(boolean noHiddenFiles) {
     final List<String> files = new ArrayList<String>();
     if (isURL) {
       try {
-        if (url == null) {
-          // Likely s3
-          if (S3Handle.canHandleScheme(uri.getScheme()) && isDirectory()) {
+        if (urlType == UrlType.S3) {
+          if (isDirectory()) {
             // TODO: This is complicated, not sure what to do here
             // See comment in isDirectory()
             LOGGER.trace("list s3 {}: Returning []", uri);
@@ -832,7 +844,7 @@ public boolean isAbsolute() {
   public boolean isDirectory() throws IOException {
     LOGGER.trace("isDirectory()");
     if (isURL) {
-      if (S3Handle.canHandleScheme(uri.getScheme())) {
+      if (urlType == UrlType.S3) {
         // TODO: This is complicated
         //
         // S3 doesn't have directories, but keys can contain / which we

From 2532d251b18e5c548748ed1bdba220e1327f4020 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 12 Oct 2018 17:18:56 +0100
Subject: [PATCH 25/55] Top parent of URLs is the server not the scheme

---
 src/main/java/loci/common/Location.java       |  7 ++
 .../java/loci/common/utests/LocationTest.java | 70 +++++++++++++++++--
 2 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 3f8932c9..c32efaa8 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -43,6 +43,8 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -796,8 +798,13 @@ public String getName() {
   public String getParent() {
     LOGGER.trace("getParent()");
     if (isURL) {
+      // TODO For S3 we should take account of directories not really existing
       String absPath = getAbsolutePath();
       absPath = absPath.substring(0, absPath.lastIndexOf("/"));
+      final Pattern pattern = Pattern.compile("[^/]+:/$");
+      if (pattern.matcher(absPath).matches()) {
+        return null;
+      }
       return absPath;
     }
     return file.getParent();
diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 72efb0d1..82ad5522 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -60,6 +60,7 @@ public class LocationTest {
   // -- Fields --
 
   private Location[] files;
+  private Location[] rootFiles;
   private boolean[] exists;
   private boolean[] isDirectory;
   private boolean[] isHidden;
@@ -93,29 +94,81 @@ public void setup() throws IOException {
       new Location("http://www.openmicroscopy.org/"),
       new Location("https://www.openmicroscopy.org/"),
       new Location("https://www.openmicroscopy.org/nonexisting"),
+      new Location("https://www.openmicroscopy.org/nonexisting/:/+/symbols"),
       new Location(hiddenFile),
       new Location("s3://server/bucket-dne/key/"),
       new Location("s3://server/bucket-dne/key/file.tif"),
     };
 
+    rootFiles = new Location[] {
+      new Location("/"),
+      new Location("https://www.openmicroscopy.org"),
+      new Location("s3://server"),
+    };
+
     exists = new boolean[] {
-      true, false, true, true, true, false, true, false, false
+      true,
+      false,
+      true,
+      true,
+      true,
+      false,
+      false,
+      true,
+      false,
+      false
     };
 
     isDirectory = new boolean[] {
-      false, false, true, false, false, false, false, false, false
+      false,
+      false,
+      true,
+      false,
+      false,
+      false,
+      false,
+      false,
+      false,
+      false
     };
 
     isHidden = new boolean[] {
-      false, false, false, false, false, false, true, false, false
+      false,
+      false,
+      false,
+      false,
+      false,
+      false,
+      false,
+      true,
+      false,
+      false
     };
 
     mode = new String[] {
-      "rw", "", "rw", "r", "r", "","rw", "", "" // S3 isn't readable
+      "rw",
+      "",
+      "rw",
+      "r",
+      "r",
+      "",
+      "",
+      "rw",
+      "",
+      "" // S3 isn't readable
     };
 
     isRemote = new boolean[] {
-      false, false, false, true, true, true, false, true, true
+      false,
+      false,
+      false,
+      true,
+      true,
+      true,
+      true,
+      false,
+      true,
+      true
     };
   }
 
@@ -179,6 +232,13 @@ public void testParent() {
     }
   }
 
+  @Test
+  public void testParentRoot() {
+    for (Location file : rootFiles) {
+      assertEquals(file.getName(), file.getParent(), null);
+    }
+  }
+
   @Test
   public void testIsDirectory() throws IOException {
     for (int i=0; i<files.length; i++) {

From b237bbb4c145d4d8b108ca82ad2962a391e7a1d5 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 19 Oct 2018 11:06:23 +0100
Subject: [PATCH 26/55] Replace Location.isDirectory IOException with
 UncheckedIOException

Avoids breaking API
---
 src/main/java/loci/common/Location.java       | 23 +++++++++++--------
 .../java/loci/common/utests/LocationTest.java | 12 +++++-----
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index c32efaa8..3140a0a4 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -9,13 +9,13 @@
  * %%
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
- * 
+ *
  * 1. Redistributions of source code must retain the above copyright notice,
  *    this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright notice,
  *    this list of conditions and the following disclaimer in the documentation
  *    and/or other materials provided with the distribution.
- * 
+ *
  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
  * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
  * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
@@ -37,6 +37,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.UncheckedIOException;
 import java.net.*;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -599,7 +600,7 @@ public String[] list(boolean noHiddenFiles) {
    * @return see above
    * @see java.io.File#canRead()
    */
-  public boolean canRead() throws IOException {
+  public boolean canRead() {
     LOGGER.trace("canRead()");
     // Note: isFile calls exist
     return isURL ? (isDirectory() || isFile()) : file.canRead();
@@ -848,7 +849,7 @@ public boolean isAbsolute() {
    * @return true if this pathname exists and represents a directory.
    * @see java.io.File#isDirectory()
    */
-  public boolean isDirectory() throws IOException {
+  public boolean isDirectory() {
     LOGGER.trace("isDirectory()");
     if (isURL) {
       if (urlType == UrlType.S3) {
@@ -864,10 +865,14 @@ public boolean isDirectory() throws IOException {
         //
         // S3 buckets are the closest thing to a proper directory
         // so for now
-        S3Handle h = new S3Handle(uri.toString(), false, null);
-        boolean isBucket = h.isBucket();
-        h.close();
-        return isBucket;
+        try {
+          S3Handle h = new S3Handle(uri.toString(), false, null);
+          boolean isBucket = h.isBucket();
+          h.close();
+          return isBucket;
+        } catch (IOException e) {
+          throw new UncheckedIOException(e);
+        }
       } else {
         // TODO: this should be removed as well.
         String[] list = list();
@@ -883,7 +888,7 @@ public boolean isDirectory() throws IOException {
    * @return true if this pathname exists and represents a regular file.
    * @see java.io.File#exists()
    */
-  public boolean isFile() throws IOException {
+  public boolean isFile() {
     LOGGER.trace("isFile()");
     return isURL ? (!isDirectory() && exists()) : file.isFile();
   }
diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 82ad5522..6fe7b25b 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -9,13 +9,13 @@
  * %%
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
- * 
+ *
  * 1. Redistributions of source code must retain the above copyright notice,
  *    this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright notice,
  *    this list of conditions and the following disclaimer in the documentation
  *    and/or other materials provided with the distribution.
- * 
+ *
  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
  * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
  * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
@@ -191,7 +191,7 @@ private void skipIfOffline(int i) throws SkipException {
   // -- Tests --
 
   @Test
-  public void testReadWriteMode() throws IOException {
+  public void testReadWriteMode() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
       String msg = files[i].getName();
@@ -240,14 +240,14 @@ public void testParentRoot() {
   }
 
   @Test
-  public void testIsDirectory() throws IOException {
+  public void testIsDirectory() {
     for (int i=0; i<files.length; i++) {
       assertEquals(files[i].getName(), files[i].isDirectory(), isDirectory[i]);
     }
   }
 
   @Test
-  public void testIsFile() throws IOException {
+  public void testIsFile() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
       assertEquals(files[i].getName(), files[i].isFile(),
@@ -263,7 +263,7 @@ public void testIsHidden() {
   }
 
   @Test
-  public void testListFiles() throws IOException {
+  public void testListFiles() {
     for (int i=0; i<files.length; i++) {
       String[] completeList = files[i].list();
       String[] unhiddenList = files[i].list(true);

From 0e3118673e2a3944dedabeb6b95eafa146ce3a66 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 19 Oct 2018 11:40:24 +0100
Subject: [PATCH 27/55] URL detection needs to handle unregistered schemes
 separately

---
 src/main/java/loci/common/Location.java | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 3140a0a4..06b70d4d 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -155,15 +155,16 @@ public Location(String parent, String child) {
         isURL = true;
         if (S3Handle.canHandleScheme(uri.toString())) {
           urlType = UrlType.S3;
+          url = null;
         }
         else {
           urlType = UrlType.GENERIC;
+          url = uri.toURL();
         }
-        url = uri.toURL();
       }
       catch (URISyntaxException | MalformedURLException e) {
-        // TODO: this should possibly throw
-        // possibly leaves url null
+        throw new UncheckedIOException(new IOException(
+          "Invalid URL: " + child, e));
       }
     }
 

From 19b5c95da764d020766d9d010413cfdeedd8686b Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 19 Oct 2018 11:47:39 +0100
Subject: [PATCH 28/55] Use static patterns for all Location regex

---
 src/main/java/loci/common/Location.java | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 06b70d4d..575ab8a1 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -98,6 +98,15 @@ protected class ListingsResult {
   private static final Map<String, ListingsResult> fileListings =
     new MapMaker().makeMap();  // like Java's ConcurrentHashMap
 
+  /** Pattern to match child URLs */
+  private static final Pattern URL_MATCHER = Pattern.compile(
+    "\\p{Alnum}+(\\+\\p{Alnum}+)?://.*");
+
+  /** Pattern to detect when getParent has gone past the parent of a URL */
+  private static final Pattern URL_ABOVE_PARENT = Pattern.compile(
+    "\\p{Alnum}+(\\+\\p{Alnum}+)?:/$");
+
+
   // -- Fields --
 
   private boolean isURL = false;
@@ -145,7 +154,7 @@ public Location(String parent, String child) {
     String pathname = null;
 
     // First handle possible URIs
-    if (child != null && child.contains("://")) {
+    if (child != null && URL_MATCHER.matcher(child).matches()) {
       // Avoid expensive exception handling in case when path is
       // obviously not an URL
       try {
@@ -803,8 +812,7 @@ public String getParent() {
       // TODO For S3 we should take account of directories not really existing
       String absPath = getAbsolutePath();
       absPath = absPath.substring(0, absPath.lastIndexOf("/"));
-      final Pattern pattern = Pattern.compile("[^/]+:/$");
-      if (pattern.matcher(absPath).matches()) {
+      if (URL_ABOVE_PARENT.matcher(absPath).matches()) {
         return null;
       }
       return absPath;

From 625ac5c5622c83090d410954fccdd2e175de3134 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 19 Oct 2018 13:44:50 +0100
Subject: [PATCH 29/55] If Location is an invalid URL ignore and treat as
 non-URL

---
 src/main/java/loci/common/Location.java | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 575ab8a1..eb25b558 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -172,8 +172,13 @@ public Location(String parent, String child) {
         }
       }
       catch (URISyntaxException | MalformedURLException e) {
-        throw new UncheckedIOException(new IOException(
-          "Invalid URL: " + child, e));
+        // Readers such as FilePatternReader may pass invalid URI paths
+        // containing <> so don't throw, instead treat as a non-URL
+        LOGGER.debug("Invalid URL: {} {}", child, e);
+        isURL = false;
+        urlType = null;
+        url = null;
+        uri = null;
       }
     }
 

From b24fa30797b23a943b446867d6564e657348f5ee Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 23 Oct 2018 15:13:49 +0100
Subject: [PATCH 30/55] Use a single MinioClient. Use getObject offset for
 large seeks.

---
 src/main/java/loci/common/Location.java |   2 +-
 src/main/java/loci/common/S3Handle.java | 185 ++++++++++++++++++------
 2 files changed, 143 insertions(+), 44 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index eb25b558..e3a6b8be 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -880,7 +880,7 @@ public boolean isDirectory() {
         // S3 buckets are the closest thing to a proper directory
         // so for now
         try {
-          S3Handle h = new S3Handle(uri.toString(), false, null);
+          S3Handle h = new S3Handle(uri.toString());
           boolean isBucket = h.isBucket();
           h.close();
           return isBucket;
diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 48ccb1cb..fd889880 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -97,10 +97,27 @@ public class S3Handle extends StreamHandle {
   /** remaining path, or key, for this accessed resource */
   private final String path;
 
+  /** Minio client */
   private MinioClient s3Client;
 
+  /** Remote file stat */
+  private ObjectStat stat;
+
+  /** Is this a directory (currently only buckets are considered directories */
+  private boolean isBucket;
+
+  /**
+   * Exception if thrown during construction
+   */
+  private Throwable objectNotFound;
+
+  /** If seeking more than this distance reset and reopen at offset */
+  protected static final int S3_MAX_FORWARD_SEEK = 1048576;
+
   /**
    * Return true if this is a URL with an s3 scheme
+   * @param url URL
+   * @return true if this class can handle url
    */
   public static boolean canHandleScheme(String url) {
     return SCHEME_PARSER.matcher(url).matches();
@@ -119,7 +136,7 @@ public S3Handle(String url) throws IOException {
   /**
    * Open an S3 file
    *
-   * @param url the full URL to the S3 resource
+   * @param uristr the full URL to the S3 resource
    * @param initialize If true open the stream, otherwise just parse connection
    *        string
    * @param s custom settings object
@@ -195,7 +212,64 @@ else if (scheme.startsWith("s3+")) {
       this.path = null;
     }
 
+    this.isBucket = false;
+    this.stat = null;
+
     if (initialize) {
+      // Throw if there is a connection error, otherwise save the exception and only throw if a method
+      // that requires a valid object is called
+      this.connect();
+      try {
+        this.initialize();
+      }
+      catch (
+        IOException |
+        MinioException |
+        InvalidKeyException |
+        NoSuchAlgorithmException |
+        XmlPullParserException e) {
+        this.objectNotFound = e;
+        LOGGER.trace("Object not found: {}", this);
+      }
+      LOGGER.trace("isBucket:{} stat:{}", isBucket, stat);
+    }
+  }
+
+  /**
+   * Connect to the server
+   * @throws IOException if there was an error connecting to the server
+   */
+  protected void connect() throws IOException {
+    try {
+      s3Client = new MinioClient(server, port, accessKey, secretKey);
+    }
+    catch (MinioException e) {
+      throw new IOException(String.format(
+              "Failed to connect: %s", this), e);
+    }
+    LOGGER.trace("connected:{}", this);
+  }
+
+  /**
+   * Check bucket or object exists
+   * @throws IOException if unable to get the object
+   * @throws MinioException if unable to get the object
+   * @throws InvalidKeyException if unable to get the object
+   * @throws NoSuchAlgorithmException if unable to get the object
+   * @throws XmlPullParserException if unable to get the object
+   */
+  protected void initialize() throws
+      IOException,
+      MinioException,
+      InvalidKeyException,
+      NoSuchAlgorithmException,
+      XmlPullParserException {
+    if (path == null) {
+      isBucket = s3Client.bucketExists(bucket);
+    }
+    else {
+      isBucket = false;
+      stat = s3Client.statObject(bucket, path);
       resetStream();
     }
   }
@@ -223,15 +297,16 @@ public String getPath() {
    * @param s custom settings object
    * @return File path to the cached object
    * @throws IOException if there is an error during reading or writing
+   * @throws HandleException if no destination for the cache is provided
    */
   public static String cacheObject(String url, Settings s) throws
       IOException,
       HandleException {
-    S3Handle s3 = new S3Handle(url, false, s);
     String cacheroot = s.getRemoteCacheRootDir();
     if (cacheroot == null) {
       throw new HandleException("Remote cache root dir is not set");
     }
+    S3Handle s3 = new S3Handle(url, true, s);
     // TODO: Need to ensure this path is safe. Is there a Java method to check?
     String cacheobj = s3.getCacheKey();
     // Hopefully creates a cross-platform path
@@ -257,17 +332,19 @@ public String getCacheKey(){
     return cachekey;
   }
 
-  protected void downloadObject(Path destination) throws HandleException {
+  protected void downloadObject(Path destination) throws HandleException, IOException {
     LOGGER.trace("destination:{}", destination);
+    if (this.objectNotFound != null) {
+      throw new IOException("Object not found", this.objectNotFound);
+    }
     if (path == null) {
       throw new HandleException("Download path=null not allowed");
     }
     try {
-      s3Client = new MinioClient(server, port, accessKey, secretKey);
-      ObjectStat stat = s3Client.statObject(bucket, path);
       Files.createDirectories(destination.getParent());
       s3Client.getObject(bucket, path, destination.toString());
-    } catch (
+    }
+    catch (
       IOException |
       InvalidKeyException |
       MinioException |
@@ -281,30 +358,60 @@ protected void downloadObject(Path destination) throws HandleException {
    * Is this an accessible bucket?
    *
    * @return True if a bucket
-   * @throws IOException if there is an error during reading or writing
    */
-  public boolean isBucket() throws IOException {
-    if (bucket == null || path !=null ) {
-      return false;
+  public boolean isBucket() {
+//    if (this.objectNotFound != null) {
+//      throw new IOException("Object not found", this.objectNotFound);
+//    }
+    return isBucket;
+  }
+
+  /* @see IRandomAccess#length() */
+  @Override
+  public long length() throws IOException {
+    if (this.objectNotFound != null) {
+      throw new IOException("Object not found", this.objectNotFound);
     }
-    try {
-      s3Client = new MinioClient(server, port, accessKey, secretKey);
-      boolean isBucket = s3Client.bucketExists(bucket);
-      LOGGER.debug("isBucket? {} {}", this, isBucket);
-      return isBucket;
-    } catch (
-      InvalidKeyException |
-      MinioException |
-      NoSuchAlgorithmException |
-      XmlPullParserException e) {
-      throw new IOException(String.format(
-              "bucketExists failed: %s", this), e);
+    return length;
+  }
+
+  /**
+   * @see StreamHandle#seek(long)
+   */
+  @Override
+  public void seek(long pos) throws IOException {
+    LOGGER.trace("{}", pos);
+    if (this.objectNotFound != null) {
+      throw new IOException("Object not found", this.objectNotFound);
+    }
+    long diff = pos - fp;
+
+    if (diff < 0 || diff > S3_MAX_FORWARD_SEEK) {
+      resetStream(pos);
+    }
+    else {
+      super.seek(pos);
     }
   }
 
+  /**
+   * @see StreamHandle#resetStream()
+   */
   @Override
   protected void resetStream() throws IOException {
-    LOGGER.trace("Resetting");
+    resetStream(0);
+  }
+
+  /**
+   * Reset the stream to an offset position
+   * @param offset Offset into object
+   * @throws IOException if there is an error during reading or writing
+   */
+  protected void resetStream(long offset) throws IOException {
+    LOGGER.trace("Resetting {}", offset);
+    if (this.objectNotFound != null) {
+      throw new IOException("Object not found", this.objectNotFound);
+    }
     if (bucket == null) {
       throw new IOException("bucket is null");
     }
@@ -312,33 +419,25 @@ protected void resetStream() throws IOException {
       throw new IOException("path is null");
     }
     try {
-      s3Client = new MinioClient(server, port, accessKey, secretKey);
-      ObjectStat stat = s3Client.statObject(bucket, path);
       length = stat.length();
       stream = new DataInputStream(new BufferedInputStream(
-              s3Client.getObject(bucket, path)));
+              s3Client.getObject(bucket, path, offset)));
       stream.skip(-1L);
-      fp = 0;
-      mark = 0;
-    } catch (ConnectException |
-            InvalidEndpointException |
-            InvalidPortException |
-            InvalidBucketNameException |
-            NoSuchAlgorithmException |
-            InsufficientDataException |
-            InvalidKeyException |
-            NoResponseException |
-            XmlPullParserException |
-            ErrorResponseException |
-            InternalException |
-            InvalidArgumentException e) {
-      throw new IOException(String.format(
+      fp = offset;
+      mark = offset;
+      }
+      catch (
+        InvalidKeyException |
+        MinioException |
+        NoSuchAlgorithmException |
+        XmlPullParserException e) {
+        throw new IOException(String.format(
               "failed to load s3: %s\n\t%s", uri, this), e);
     }
   }
 
   public String toString() {
-    return String.format("server:%s port:%d bucket:%s path:%s",
-                          server, port, bucket, path);
+    return String.format("server:%s port:%d bucket:%s path:%s found:%s",
+                          server, port, bucket, path, objectNotFound == null);
   }
 }

From f44ed189bd0f1349a21fdbbcefd6ad1b71e9adfc Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 23 Oct 2018 15:20:36 +0100
Subject: [PATCH 31/55] Update LocationTest including real minio/s3 server

Print warning if online or s3 tests are skipped
---
 .../java/loci/common/utests/LocationTest.java | 76 ++++++++++++++-----
 1 file changed, 56 insertions(+), 20 deletions(-)

diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 6fe7b25b..61baa3f1 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -59,14 +59,21 @@ public class LocationTest {
 
   // -- Fields --
 
+  private enum LocalRemoteType {
+    LOCAL,
+    HTTP,
+    S3,
+  };
+
   private Location[] files;
   private Location[] rootFiles;
   private boolean[] exists;
   private boolean[] isDirectory;
   private boolean[] isHidden;
   private String[] mode;
-  private boolean[] isRemote;
+  private LocalRemoteType[] isRemote;
   private boolean isOnline;
+  private boolean canAccessS3;
 
   // -- Setup methods --
 
@@ -96,14 +103,15 @@ public void setup() throws IOException {
       new Location("https://www.openmicroscopy.org/nonexisting"),
       new Location("https://www.openmicroscopy.org/nonexisting/:/+/symbols"),
       new Location(hiddenFile),
-      new Location("s3://server/bucket-dne/key/"),
-      new Location("s3://server/bucket-dne/key/file.tif"),
+      new Location("s3://minio.openmicroscopy.org/bucket-dne"),
+      new Location("s3://minio.openmicroscopy.org/bioformats.test.public"),
+      new Location("s3://minio.openmicroscopy.org/bioformats.test.public/z-series.ome.tif"),
     };
 
     rootFiles = new Location[] {
       new Location("/"),
       new Location("https://www.openmicroscopy.org"),
-      new Location("s3://server"),
+      new Location("s3://minio.openmicroscopy.org"),
     };
 
     exists = new boolean[] {
@@ -116,7 +124,8 @@ public void setup() throws IOException {
       false,
       true,
       false,
-      false
+      true,
+      true,
     };
 
     isDirectory = new boolean[] {
@@ -129,7 +138,8 @@ public void setup() throws IOException {
       false,
       false,
       false,
-      false
+      true,
+      false,
     };
 
     isHidden = new boolean[] {
@@ -142,7 +152,8 @@ public void setup() throws IOException {
       false,
       true,
       false,
-      false
+      false,
+      false,
     };
 
     mode = new String[] {
@@ -155,20 +166,22 @@ public void setup() throws IOException {
       "",
       "rw",
       "",
-      "" // S3 isn't readable
+      "r",
+      "r",
     };
 
-    isRemote = new boolean[] {
-      false,
-      false,
-      false,
-      true,
-      true,
-      true,
-      true,
-      false,
-      true,
-      true
+    isRemote = new LocalRemoteType[] {
+      LocalRemoteType.LOCAL,
+      LocalRemoteType.LOCAL,
+      LocalRemoteType.LOCAL,
+      LocalRemoteType.HTTP,
+      LocalRemoteType.HTTP,
+      LocalRemoteType.HTTP,
+      LocalRemoteType.HTTP,
+      LocalRemoteType.LOCAL,
+      LocalRemoteType.S3,
+      LocalRemoteType.S3,
+      LocalRemoteType.S3,
     };
   }
 
@@ -180,20 +193,40 @@ public void checkIfOnline() throws IOException {
     } catch (IOException e) {
       isOnline = false;
     }
+    try {
+      new Socket("minio.openmicroscopy.org", 443).close();
+      canAccessS3 = true;
+    } catch (IOException e) {
+      canAccessS3 = false;
+    }
+
+    if (!isOnline) {
+      System.err.println("WARNING: online tests are disabled!");
+    }
+    if (!canAccessS3) {
+      System.err.println("WARNING: S3 tests are disabled!");
+    }
   }
 
   private void skipIfOffline(int i) throws SkipException {
-    if (isRemote[i] && !isOnline) {
+    if (isRemote[i] == LocalRemoteType.HTTP && !isOnline) {
       throw new SkipException("must be online to test " + files[i].getName());
     }
   }
 
+  private void skipIfS3Offline(int i) throws SkipException {
+    if (isRemote[i] == LocalRemoteType.S3 && !canAccessS3) {
+      throw new SkipException("must have access to s3 to test " + files[i].getName());
+    }
+  }
+
   // -- Tests --
 
   @Test
   public void testReadWriteMode() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
+      skipIfS3Offline(i);
       String msg = files[i].getName();
       assertEquals(msg, files[i].canRead(), mode[i].contains("r"));
       assertEquals(msg, files[i].canWrite(), mode[i].contains("w"));
@@ -212,6 +245,7 @@ public void testAbsolute() {
   public void testExists() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
+      skipIfS3Offline(i);
       assertEquals(files[i].getName(), files[i].exists(), exists[i]);
     }
   }
@@ -242,6 +276,7 @@ public void testParentRoot() {
   @Test
   public void testIsDirectory() {
     for (int i=0; i<files.length; i++) {
+      skipIfS3Offline(i);
       assertEquals(files[i].getName(), files[i].isDirectory(), isDirectory[i]);
     }
   }
@@ -250,6 +285,7 @@ public void testIsDirectory() {
   public void testIsFile() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
+      skipIfS3Offline(i);
       assertEquals(files[i].getName(), files[i].isFile(),
         !isDirectory[i] && exists[i]);
     }

From e56a65bba8622aae50ee437a17d7ea6b15f54c45 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 24 Oct 2018 12:01:15 +0100
Subject: [PATCH 32/55] S3 throw if object not found or stat is null

---
 src/main/java/loci/common/S3Handle.java | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index fd889880..6dd8262d 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -381,7 +381,7 @@ public long length() throws IOException {
   @Override
   public void seek(long pos) throws IOException {
     LOGGER.trace("{}", pos);
-    if (this.objectNotFound != null) {
+    if (this.stat == null || this.objectNotFound != null) {
       throw new IOException("Object not found", this.objectNotFound);
     }
     long diff = pos - fp;
@@ -409,15 +409,9 @@ protected void resetStream() throws IOException {
    */
   protected void resetStream(long offset) throws IOException {
     LOGGER.trace("Resetting {}", offset);
-    if (this.objectNotFound != null) {
+    if (this.stat == null || this.objectNotFound != null) {
       throw new IOException("Object not found", this.objectNotFound);
     }
-    if (bucket == null) {
-      throw new IOException("bucket is null");
-    }
-    if (path == null) {
-      throw new IOException("path is null");
-    }
     try {
       length = stat.length();
       stream = new DataInputStream(new BufferedInputStream(
@@ -437,7 +431,8 @@ protected void resetStream(long offset) throws IOException {
   }
 
   public String toString() {
+    boolean found = (objectNotFound == null) && (isBucket || stat != null);
     return String.format("server:%s port:%d bucket:%s path:%s found:%s",
-                          server, port, bucket, path, objectNotFound == null);
+                          server, port, bucket, path, found);
   }
 }

From 4dd0eefae8377b2ad6cccd4bf191a280e7232cc3 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 24 Oct 2018 14:47:29 +0100
Subject: [PATCH 33/55] exists() for URLs is handle by the handles not Location

Add more info when object not found is thrown
---
 src/main/java/loci/common/Location.java     |  7 +++--
 src/main/java/loci/common/S3Handle.java     | 29 ++++++++++++++-------
 src/main/java/loci/common/StreamHandle.java | 11 ++++++++
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index e3a6b8be..2307417c 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -722,10 +722,9 @@ public boolean exists() {
     if (isURL) {
       try {
         // TODO: existence should almost certainly be cached.
-        IRandomAccess handle = getHandle(uri.toString());
-        handle.length();
-        handle.close();
-        return true;
+        StreamHandle handle = (StreamHandle) getHandle(uri.toString());
+        boolean exists = handle.exists();
+        return exists;
       }
       catch (IOException e) {
         LOGGER.trace("Failed to retrieve content from URL", e);
diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 6dd8262d..78d558a4 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -334,8 +334,8 @@ public String getCacheKey(){
 
   protected void downloadObject(Path destination) throws HandleException, IOException {
     LOGGER.trace("destination:{}", destination);
-    if (this.objectNotFound != null) {
-      throw new IOException("Object not found", this.objectNotFound);
+    if (this.stat == null || this.objectNotFound != null) {
+      throw new IOException("Object not found " + this, this.objectNotFound);
     }
     if (path == null) {
       throw new HandleException("Download path=null not allowed");
@@ -356,21 +356,22 @@ protected void downloadObject(Path destination) throws HandleException, IOExcept
 
   /**
    * Is this an accessible bucket?
+   * TODO: If this bucket doesn't exist do we return false or thrown an exception?
    *
    * @return True if a bucket
    */
   public boolean isBucket() {
-//    if (this.objectNotFound != null) {
-//      throw new IOException("Object not found", this.objectNotFound);
-//    }
+    //if (this.objectNotFound != null) {
+    //  throw new IOException("Object not found " + this, this.objectNotFound);
+    //}
     return isBucket;
   }
 
   /* @see IRandomAccess#length() */
   @Override
   public long length() throws IOException {
-    if (this.objectNotFound != null) {
-      throw new IOException("Object not found", this.objectNotFound);
+    if (this.stat == null || this.objectNotFound != null) {
+      throw new IOException("Object not found " + this, this.objectNotFound);
     }
     return length;
   }
@@ -382,7 +383,7 @@ public long length() throws IOException {
   public void seek(long pos) throws IOException {
     LOGGER.trace("{}", pos);
     if (this.stat == null || this.objectNotFound != null) {
-      throw new IOException("Object not found", this.objectNotFound);
+      throw new IOException("Object not found " + this, this.objectNotFound);
     }
     long diff = pos - fp;
 
@@ -402,6 +403,16 @@ protected void resetStream() throws IOException {
     resetStream(0);
   }
 
+  /**
+   * Does this represent an accessible location?
+   * @return true if this location is accessible
+   * @throws IOException if unable to determine whether this location is accessible
+   */
+  @Override
+  public boolean exists() throws IOException {
+    return (objectNotFound == null) && (isBucket || stat != null);
+  }
+
   /**
    * Reset the stream to an offset position
    * @param offset Offset into object
@@ -410,7 +421,7 @@ protected void resetStream() throws IOException {
   protected void resetStream(long offset) throws IOException {
     LOGGER.trace("Resetting {}", offset);
     if (this.stat == null || this.objectNotFound != null) {
-      throw new IOException("Object not found", this.objectNotFound);
+      throw new IOException("Object not found " + this, this.objectNotFound);
     }
     try {
       length = stat.length();
diff --git a/src/main/java/loci/common/StreamHandle.java b/src/main/java/loci/common/StreamHandle.java
index 2200092a..a3f785f5 100644
--- a/src/main/java/loci/common/StreamHandle.java
+++ b/src/main/java/loci/common/StreamHandle.java
@@ -478,6 +478,17 @@ public void writeUTF(String str) throws IOException {
     outStream.writeUTF(str);
   }
 
+  // -- Public methods --
+
+  /**
+   * Does this represent an accessible location?
+   * @return true if this location is accessible
+   * @throws IOException if unable to determine whether this location is accessible
+   */
+  public boolean exists() throws IOException {
+    return length >= 0;
+  }
+
   // -- Helper methods --
 
   /**

From b5c0c47518a62adb79f20563a68f217dd61efc9c Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 24 Oct 2018 11:09:59 +0100
Subject: [PATCH 34/55] Add docker minio for s3 tests

---
 .travis.yml                                        |  4 ++++
 location-server/Dockerfile                         | 12 ++++++++++++
 location-server/initialise.sh                      | 14 ++++++++++++++
 src/test/java/loci/common/utests/LocationTest.java | 12 ++++++------
 4 files changed, 36 insertions(+), 6 deletions(-)
 create mode 100644 location-server/Dockerfile
 create mode 100755 location-server/initialise.sh

diff --git a/.travis.yml b/.travis.yml
index 1cc9c60c..973d9778 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -11,5 +11,9 @@ jdk:
   - oraclejdk11
   - oraclejdk8
 
+before_script:
+  - docker build -t location-server location-server/
+  - docker run -d --name location-server -p 31836:9000 location-server
+
 matrix:
   fast_finish: true
diff --git a/location-server/Dockerfile b/location-server/Dockerfile
new file mode 100644
index 00000000..55d82b1c
--- /dev/null
+++ b/location-server/Dockerfile
@@ -0,0 +1,12 @@
+FROM minio/minio:RELEASE.2018-10-18T00-28-58Z
+MAINTAINER ome-devel@lists.openmicroscopy.org.uk
+
+RUN wget https://dl.minio.io/client/mc/release/linux-amd64/mc -qO /usr/bin/mc && \
+    chmod a+x /usr/bin/mc
+ENV MINIO_ACCESS_KEY=accesskey
+ENV MINIO_SECRET_KEY=secretkey
+
+ADD initialise.sh /srv/
+RUN /srv/initialise.sh
+
+CMD ["server", "/srv"]
diff --git a/location-server/initialise.sh b/location-server/initialise.sh
new file mode 100755
index 00000000..786c7c9d
--- /dev/null
+++ b/location-server/initialise.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+set -eux
+
+minio server /srv &
+sleep 2;
+
+mc config host add minio http://localhost:9000 ${MINIO_ACCESS_KEY} ${MINIO_SECRET_KEY}
+
+for SUFFIX in public private; do
+  mc mb minio/bioformats.test.$SUFFIX
+  wget http://downloads.openmicroscopy.org/images/OME-TIFF/2016-06/bioformats-artificial/single-channel.ome.tiff -O - | mc pipe minio/bioformats.test.$SUFFIX/single-channel.ome.tiff
+done
+
+mc policy public minio/bioformats.test.public
diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 61baa3f1..6403d119 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -103,15 +103,15 @@ public void setup() throws IOException {
       new Location("https://www.openmicroscopy.org/nonexisting"),
       new Location("https://www.openmicroscopy.org/nonexisting/:/+/symbols"),
       new Location(hiddenFile),
-      new Location("s3://minio.openmicroscopy.org/bucket-dne"),
-      new Location("s3://minio.openmicroscopy.org/bioformats.test.public"),
-      new Location("s3://minio.openmicroscopy.org/bioformats.test.public/z-series.ome.tif"),
+      new Location("s3+http://localhost:31836/bucket-dne"),
+      new Location("s3+http://localhost:31836/bioformats.test.public"),
+      new Location("s3+http://localhost:31836/bioformats.test.public/single-channel.ome.tiff"),
     };
 
     rootFiles = new Location[] {
       new Location("/"),
       new Location("https://www.openmicroscopy.org"),
-      new Location("s3://minio.openmicroscopy.org"),
+      new Location("s3://s3.example.org"),
     };
 
     exists = new boolean[] {
@@ -194,7 +194,7 @@ public void checkIfOnline() throws IOException {
       isOnline = false;
     }
     try {
-      new Socket("minio.openmicroscopy.org", 443).close();
+      new Socket("localhost", 31836).close();
       canAccessS3 = true;
     } catch (IOException e) {
       canAccessS3 = false;
@@ -346,7 +346,7 @@ public void testToURL() throws IOException {
       try {
         assertEquals(file.getName(), file.toURL(), new URL(path));
       } catch (MalformedURLException e) {
-        assertEquals(path, true, path.contains("s3://"));
+        assertEquals(path, true, path.contains("s3+http://"));
       }
     }
   }

From ce68184de6354da31f131b77dcaa0571b350cbdd Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 24 Oct 2018 11:13:59 +0100
Subject: [PATCH 35/55] s3 test private bucket

---
 src/test/java/loci/common/utests/LocationTest.java | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 6403d119..8b4f6543 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -106,6 +106,8 @@ public void setup() throws IOException {
       new Location("s3+http://localhost:31836/bucket-dne"),
       new Location("s3+http://localhost:31836/bioformats.test.public"),
       new Location("s3+http://localhost:31836/bioformats.test.public/single-channel.ome.tiff"),
+      new Location("s3+http://localhost:31836/bioformats.test.private/single-channel.ome.tiff"),
+      new Location("s3+http://accesskey:secretkey@localhost:31836/bioformats.test.private/single-channel.ome.tiff")
     };
 
     rootFiles = new Location[] {
@@ -126,6 +128,8 @@ public void setup() throws IOException {
       false,
       true,
       true,
+      false,
+      true,
     };
 
     isDirectory = new boolean[] {
@@ -140,6 +144,8 @@ public void setup() throws IOException {
       false,
       true,
       false,
+      false,
+      false,
     };
 
     isHidden = new boolean[] {
@@ -154,6 +160,8 @@ public void setup() throws IOException {
       false,
       false,
       false,
+      false,
+      false,
     };
 
     mode = new String[] {
@@ -168,6 +176,8 @@ public void setup() throws IOException {
       "",
       "r",
       "r",
+      "",
+      "r",
     };
 
     isRemote = new LocalRemoteType[] {
@@ -182,6 +192,8 @@ public void setup() throws IOException {
       LocalRemoteType.S3,
       LocalRemoteType.S3,
       LocalRemoteType.S3,
+      LocalRemoteType.S3,
+      LocalRemoteType.S3,
     };
   }
 

From 0255fa9e09654da17e8774b75524f0053a3319f8 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 24 Oct 2018 11:20:46 +0100
Subject: [PATCH 36/55] Fix order of assertEquals parameters

Should be `assertEquals(message, expected, actual)`, expected/actual were switched around
---
 .../java/loci/common/utests/LocationTest.java | 45 +++++++++----------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 8b4f6543..a6601fb5 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -233,6 +233,7 @@ private void skipIfS3Offline(int i) throws SkipException {
   }
 
   // -- Tests --
+  // Order of assertEquals parameters is assertEquals(message, expected, actual)
 
   @Test
   public void testReadWriteMode() {
@@ -240,16 +241,15 @@ public void testReadWriteMode() {
       skipIfOffline(i);
       skipIfS3Offline(i);
       String msg = files[i].getName();
-      assertEquals(msg, files[i].canRead(), mode[i].contains("r"));
-      assertEquals(msg, files[i].canWrite(), mode[i].contains("w"));
+      assertEquals(msg, mode[i].contains("r"), files[i].canRead());
+      assertEquals(msg, mode[i].contains("w"), files[i].canWrite());
     }
   }
 
   @Test
   public void testAbsolute() {
     for (Location file : files) {
-      assertEquals(file.getName(), file.getAbsolutePath(),
-        file.getAbsoluteFile().getAbsolutePath());
+      assertEquals(file.getName(), file.getAbsoluteFile().getAbsolutePath(), file.getAbsolutePath());
     }
   }
 
@@ -258,30 +258,28 @@ public void testExists() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
       skipIfS3Offline(i);
-      assertEquals(files[i].getName(), files[i].exists(), exists[i]);
+      assertEquals(files[i].getName(), exists[i], files[i].exists());
     }
   }
 
   @Test
   public void testCanonical() throws IOException {
     for (Location file : files) {
-      assertEquals(file.getName(), file.getCanonicalPath(),
-        file.getCanonicalFile().getAbsolutePath());
+      assertEquals(file.getName(), file.getCanonicalFile().getAbsolutePath(), file.getCanonicalPath());
     }
   }
 
   @Test
   public void testParent() {
     for (Location file : files) {
-      assertEquals(file.getName(), file.getParent(),
-        file.getParentFile().getAbsolutePath());
+      assertEquals(file.getName(), file.getParentFile().getAbsolutePath(), file.getParent());
     }
   }
 
   @Test
   public void testParentRoot() {
     for (Location file : rootFiles) {
-      assertEquals(file.getName(), file.getParent(), null);
+      assertEquals(file.getName(), null, file.getParent());
     }
   }
 
@@ -289,7 +287,7 @@ public void testParentRoot() {
   public void testIsDirectory() {
     for (int i=0; i<files.length; i++) {
       skipIfS3Offline(i);
-      assertEquals(files[i].getName(), files[i].isDirectory(), isDirectory[i]);
+      assertEquals(files[i].getName(), isDirectory[i], files[i].isDirectory());
     }
   }
 
@@ -298,15 +296,14 @@ public void testIsFile() {
     for (int i=0; i<files.length; i++) {
       skipIfOffline(i);
       skipIfS3Offline(i);
-      assertEquals(files[i].getName(), files[i].isFile(),
-        !isDirectory[i] && exists[i]);
+      assertEquals(files[i].getName(), !isDirectory[i] && exists[i], files[i].isFile());
     }
   }
 
   @Test
   public void testIsHidden() {
     for (int i=0; i<files.length; i++) {
-      assertEquals(files[i].getName(), files[i].isHidden() || IS_WINDOWS, isHidden[i] || IS_WINDOWS);
+      assertEquals(files[i].getName(), isHidden[i] || IS_WINDOWS, files[i].isHidden() || IS_WINDOWS);
     }
   }
 
@@ -318,24 +315,22 @@ public void testListFiles() {
       Location[] fileList = files[i].listFiles();
 
       if (!files[i].isDirectory()) {
-        assertEquals(files[i].getName(), completeList, null);
-        assertEquals(files[i].getName(), unhiddenList, null);
-        assertEquals(files[i].getName(), fileList, null);
+        assertEquals(files[i].getName(), null, completeList);
+        assertEquals(files[i].getName(), null, unhiddenList);
+        assertEquals(files[i].getName(), null, fileList);
         continue;
       }
 
-      assertEquals(files[i].getName(), completeList.length, fileList.length);
+      assertEquals(files[i].getName(), fileList.length, completeList.length);
 
       List<String> complete = Arrays.asList(completeList);
       for (String child : unhiddenList) {
-        assertEquals(files[i].getName(), complete.contains(child), true);
-        assertEquals(files[i].getName(),
-          new Location(files[i], child).isHidden(), false);
+        assertEquals(files[i].getName(), true, complete.contains(child));
+        assertEquals(files[i].getName(), false, new Location(files[i], child).isHidden());
       }
 
       for (int f=0; f<fileList.length; f++) {
-        assertEquals(files[i].getName(),
-          fileList[f].getName(), completeList[f]);
+        assertEquals(files[i].getName(), completeList[f], fileList[f].getName());
       }
     }
   }
@@ -356,7 +351,7 @@ public void testToURL() throws IOException {
         path += File.separator;
       }
       try {
-        assertEquals(file.getName(), file.toURL(), new URL(path));
+        assertEquals(file.getName(), new URL(path), file.toURL());
       } catch (MalformedURLException e) {
         assertEquals(path, true, path.contains("s3+http://"));
       }
@@ -366,7 +361,7 @@ public void testToURL() throws IOException {
   @Test
   public void testToString() {
     for (Location file : files) {
-      assertEquals(file.getName(), file.toString(), file.getAbsolutePath());
+      assertEquals(file.getName(), file.getAbsolutePath(), file.toString());
     }
   }
 

From 6f95f6337c53fd65cf0fd53961d88bd5c8654e9e Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 25 Oct 2018 11:37:11 +0100
Subject: [PATCH 37/55] close handle in Location.exists()

---
 src/main/java/loci/common/Location.java | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 2307417c..6b3d4473 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -724,6 +724,7 @@ public boolean exists() {
         // TODO: existence should almost certainly be cached.
         StreamHandle handle = (StreamHandle) getHandle(uri.toString());
         boolean exists = handle.exists();
+        handle.close();
         return exists;
       }
       catch (IOException e) {

From 3f679ade7d1bee4e69ae8db7563cbeb8666c8123 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 30 Oct 2018 11:38:10 +0000
Subject: [PATCH 38/55] Replace docker minio server with local

---
 .travis.yml                       |  3 +--
 location-server/.gitignore        |  5 +++++
 location-server/Dockerfile        | 12 ------------
 location-server/initialise.sh     | 14 --------------
 location-server/start-location.sh | 29 +++++++++++++++++++++++++++++
 5 files changed, 35 insertions(+), 28 deletions(-)
 create mode 100644 location-server/.gitignore
 delete mode 100644 location-server/Dockerfile
 delete mode 100755 location-server/initialise.sh
 create mode 100755 location-server/start-location.sh

diff --git a/.travis.yml b/.travis.yml
index 973d9778..6f8fbf08 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -12,8 +12,7 @@ jdk:
   - oraclejdk8
 
 before_script:
-  - docker build -t location-server location-server/
-  - docker run -d --name location-server -p 31836:9000 location-server
+  - cd location-server && ./start-location.sh && cd ..
 
 matrix:
   fast_finish: true
diff --git a/location-server/.gitignore b/location-server/.gitignore
new file mode 100644
index 00000000..476165b5
--- /dev/null
+++ b/location-server/.gitignore
@@ -0,0 +1,5 @@
+bioformats.test.private
+bioformats.test.public
+mc
+minio
+.minio.sys
diff --git a/location-server/Dockerfile b/location-server/Dockerfile
deleted file mode 100644
index 55d82b1c..00000000
--- a/location-server/Dockerfile
+++ /dev/null
@@ -1,12 +0,0 @@
-FROM minio/minio:RELEASE.2018-10-18T00-28-58Z
-MAINTAINER ome-devel@lists.openmicroscopy.org.uk
-
-RUN wget https://dl.minio.io/client/mc/release/linux-amd64/mc -qO /usr/bin/mc && \
-    chmod a+x /usr/bin/mc
-ENV MINIO_ACCESS_KEY=accesskey
-ENV MINIO_SECRET_KEY=secretkey
-
-ADD initialise.sh /srv/
-RUN /srv/initialise.sh
-
-CMD ["server", "/srv"]
diff --git a/location-server/initialise.sh b/location-server/initialise.sh
deleted file mode 100755
index 786c7c9d..00000000
--- a/location-server/initialise.sh
+++ /dev/null
@@ -1,14 +0,0 @@
-#!/bin/sh
-set -eux
-
-minio server /srv &
-sleep 2;
-
-mc config host add minio http://localhost:9000 ${MINIO_ACCESS_KEY} ${MINIO_SECRET_KEY}
-
-for SUFFIX in public private; do
-  mc mb minio/bioformats.test.$SUFFIX
-  wget http://downloads.openmicroscopy.org/images/OME-TIFF/2016-06/bioformats-artificial/single-channel.ome.tiff -O - | mc pipe minio/bioformats.test.$SUFFIX/single-channel.ome.tiff
-done
-
-mc policy public minio/bioformats.test.public
diff --git a/location-server/start-location.sh b/location-server/start-location.sh
new file mode 100755
index 00000000..040abf84
--- /dev/null
+++ b/location-server/start-location.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+set -eux
+
+PORT=31836
+PLATFORM=`uname | tr '[:upper:]' '[:lower:]'`
+
+[ -f minio ] || \
+    curl -sfSo minio "https://dl.minio.io/server/minio/release/$PLATFORM-amd64/minio"
+[ -f mc ] || \
+    curl -sfSo mc "https://dl.minio.io/client/mc/release/$PLATFORM-amd64/mc"
+chmod +x minio mc
+./minio version
+./mc version
+
+export MINIO_ACCESS_KEY=accesskey MINIO_SECRET_KEY=secretkey
+./minio server --address localhost:$PORT . &
+sleep 2;
+
+./mc config host add ome-common-java-minio-test http://localhost:$PORT ${MINIO_ACCESS_KEY} ${MINIO_SECRET_KEY}
+
+for SUFFIX in public private; do
+    ./mc ls ome-common-java-minio-test/bioformats.test.$SUFFIX || \
+        ./mc mb ome-common-java-minio-test/bioformats.test.$SUFFIX
+    ./mc ls ome-common-java-minio-test/bioformats.test.$SUFFIX/single-channel.ome.tiff || \
+        curl -sfS https://downloads.openmicroscopy.org/images/OME-TIFF/2016-06/bioformats-artificial/single-channel.ome.tiff | \
+            ./mc pipe ome-common-java-minio-test/bioformats.test.$SUFFIX/single-channel.ome.tiff
+done
+
+./mc policy public ome-common-java-minio-test/bioformats.test.public

From 47b84a4c8720d57908b469b4d1fd916daa3814fe Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 30 Oct 2018 11:42:30 +0000
Subject: [PATCH 39/55] Make url and s3 tests mandatory

---
 .../java/loci/common/utests/LocationTest.java | 29 +++++++++----------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index a6601fb5..8af22c8b 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -33,6 +33,7 @@
 package loci.common.utests;
 
 import static org.testng.AssertJUnit.assertEquals;
+import static org.testng.AssertJUnit.assertFalse;
 
 import java.io.File;
 import java.io.IOException;
@@ -220,16 +221,14 @@ public void checkIfOnline() throws IOException {
     }
   }
 
-  private void skipIfOffline(int i) throws SkipException {
-    if (isRemote[i] == LocalRemoteType.HTTP && !isOnline) {
-      throw new SkipException("must be online to test " + files[i].getName());
-    }
+  private void failIfOffline(int i) throws SkipException {
+    assertFalse("must be online to test " + files[i].getName(),
+       isRemote[i] == LocalRemoteType.HTTP && !isOnline);
   }
 
-  private void skipIfS3Offline(int i) throws SkipException {
-    if (isRemote[i] == LocalRemoteType.S3 && !canAccessS3) {
-      throw new SkipException("must have access to s3 to test " + files[i].getName());
-    }
+  private void failIfS3Offline(int i) throws SkipException {
+    assertFalse("Server not found, run `cd location-server && ./start-location.sh`: " + files[i].getName(),
+      isRemote[i] == LocalRemoteType.S3 && !canAccessS3);
   }
 
   // -- Tests --
@@ -238,8 +237,8 @@ private void skipIfS3Offline(int i) throws SkipException {
   @Test
   public void testReadWriteMode() {
     for (int i=0; i<files.length; i++) {
-      skipIfOffline(i);
-      skipIfS3Offline(i);
+      failIfOffline(i);
+      failIfS3Offline(i);
       String msg = files[i].getName();
       assertEquals(msg, mode[i].contains("r"), files[i].canRead());
       assertEquals(msg, mode[i].contains("w"), files[i].canWrite());
@@ -256,8 +255,8 @@ public void testAbsolute() {
   @Test
   public void testExists() {
     for (int i=0; i<files.length; i++) {
-      skipIfOffline(i);
-      skipIfS3Offline(i);
+      failIfOffline(i);
+      failIfS3Offline(i);
       assertEquals(files[i].getName(), exists[i], files[i].exists());
     }
   }
@@ -286,7 +285,7 @@ public void testParentRoot() {
   @Test
   public void testIsDirectory() {
     for (int i=0; i<files.length; i++) {
-      skipIfS3Offline(i);
+      failIfS3Offline(i);
       assertEquals(files[i].getName(), isDirectory[i], files[i].isDirectory());
     }
   }
@@ -294,8 +293,8 @@ public void testIsDirectory() {
   @Test
   public void testIsFile() {
     for (int i=0; i<files.length; i++) {
-      skipIfOffline(i);
-      skipIfS3Offline(i);
+      failIfOffline(i);
+      failIfS3Offline(i);
       assertEquals(files[i].getName(), !isDirectory[i] && exists[i], files[i].isFile());
     }
   }

From d72bd1ff46992e0241b119fc7e1d30cc0a287603 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 31 Oct 2018 15:38:14 +0000
Subject: [PATCH 40/55] Revert "Make url and s3 tests mandatory"

This reverts commit 47b84a4c8720d57908b469b4d1fd916daa3814fe.
---
 .../java/loci/common/utests/LocationTest.java | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 8af22c8b..a6601fb5 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -33,7 +33,6 @@
 package loci.common.utests;
 
 import static org.testng.AssertJUnit.assertEquals;
-import static org.testng.AssertJUnit.assertFalse;
 
 import java.io.File;
 import java.io.IOException;
@@ -221,14 +220,16 @@ public void checkIfOnline() throws IOException {
     }
   }
 
-  private void failIfOffline(int i) throws SkipException {
-    assertFalse("must be online to test " + files[i].getName(),
-       isRemote[i] == LocalRemoteType.HTTP && !isOnline);
+  private void skipIfOffline(int i) throws SkipException {
+    if (isRemote[i] == LocalRemoteType.HTTP && !isOnline) {
+      throw new SkipException("must be online to test " + files[i].getName());
+    }
   }
 
-  private void failIfS3Offline(int i) throws SkipException {
-    assertFalse("Server not found, run `cd location-server && ./start-location.sh`: " + files[i].getName(),
-      isRemote[i] == LocalRemoteType.S3 && !canAccessS3);
+  private void skipIfS3Offline(int i) throws SkipException {
+    if (isRemote[i] == LocalRemoteType.S3 && !canAccessS3) {
+      throw new SkipException("must have access to s3 to test " + files[i].getName());
+    }
   }
 
   // -- Tests --
@@ -237,8 +238,8 @@ private void failIfS3Offline(int i) throws SkipException {
   @Test
   public void testReadWriteMode() {
     for (int i=0; i<files.length; i++) {
-      failIfOffline(i);
-      failIfS3Offline(i);
+      skipIfOffline(i);
+      skipIfS3Offline(i);
       String msg = files[i].getName();
       assertEquals(msg, mode[i].contains("r"), files[i].canRead());
       assertEquals(msg, mode[i].contains("w"), files[i].canWrite());
@@ -255,8 +256,8 @@ public void testAbsolute() {
   @Test
   public void testExists() {
     for (int i=0; i<files.length; i++) {
-      failIfOffline(i);
-      failIfS3Offline(i);
+      skipIfOffline(i);
+      skipIfS3Offline(i);
       assertEquals(files[i].getName(), exists[i], files[i].exists());
     }
   }
@@ -285,7 +286,7 @@ public void testParentRoot() {
   @Test
   public void testIsDirectory() {
     for (int i=0; i<files.length; i++) {
-      failIfS3Offline(i);
+      skipIfS3Offline(i);
       assertEquals(files[i].getName(), isDirectory[i], files[i].isDirectory());
     }
   }
@@ -293,8 +294,8 @@ public void testIsDirectory() {
   @Test
   public void testIsFile() {
     for (int i=0; i<files.length; i++) {
-      failIfOffline(i);
-      failIfS3Offline(i);
+      skipIfOffline(i);
+      skipIfS3Offline(i);
       assertEquals(files[i].getName(), !isDirectory[i] && exists[i], files[i].isFile());
     }
   }

From e7d951cbd37f837a8677a8e4e974f60d4d0e736a Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 31 Oct 2018 16:17:59 +0000
Subject: [PATCH 41/55] Disable HTTP and S3 tests by default

Enabled by setting properties testng.runHttpRemoteTests testng.runS3RemoteTests
---
 .travis.yml                                   |  3 ++
 pom.xml                                       |  4 ++
 .../java/loci/common/utests/LocationTest.java | 53 ++++++++-----------
 .../loci/common/utests/TestUtilities.java     | 51 ++++++++++++++++++
 4 files changed, 79 insertions(+), 32 deletions(-)
 create mode 100644 src/test/java/loci/common/utests/TestUtilities.java

diff --git a/.travis.yml b/.travis.yml
index 6f8fbf08..3373b4ff 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -14,5 +14,8 @@ jdk:
 before_script:
   - cd location-server && ./start-location.sh && cd ..
 
+script:
+  - mvn test -B -Dtestng.runHttpRemoteTests=1 -Dtestng.runS3RemoteTests=1
+
 matrix:
   fast_finish: true
diff --git a/pom.xml b/pom.xml
index 4c9d61b7..64892afb 100644
--- a/pom.xml
+++ b/pom.xml
@@ -238,6 +238,10 @@
           <suiteXmlFiles>
             <suiteXmlFile>src/test/java/loci/common/utests/testng-template.xml</suiteXmlFile>
           </suiteXmlFiles>
+          <systemPropertyVariables>
+            <testng.runHttpRemoteTests>0</testng.runHttpRemoteTests>
+            <testng.runS3RemoteTests>0</testng.runS3RemoteTests>
+          </systemPropertyVariables>
         </configuration>
       </plugin>
 
diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index a6601fb5..11a1eab7 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -37,7 +37,6 @@
 import java.io.File;
 import java.io.IOException;
 import java.net.MalformedURLException;
-import java.net.Socket;
 import java.net.URL;
 import java.util.Arrays;
 import java.util.List;
@@ -72,8 +71,8 @@ private enum LocalRemoteType {
   private boolean[] isHidden;
   private String[] mode;
   private LocalRemoteType[] isRemote;
-  private boolean isOnline;
-  private boolean canAccessS3;
+  private static boolean runHttpRemoteTests;
+  private static boolean runS3RemoteTests;
 
   // -- Setup methods --
 
@@ -198,37 +197,27 @@ public void setup() throws IOException {
   }
 
   @BeforeClass
-  public void checkIfOnline() throws IOException {
-    try {
-      new Socket("www.openmicroscopy.org", 80).close();
-      isOnline = true;
-    } catch (IOException e) {
-      isOnline = false;
-    }
-    try {
-      new Socket("localhost", 31836).close();
-      canAccessS3 = true;
-    } catch (IOException e) {
-      canAccessS3 = false;
-    }
+  public void checkProperties() throws IOException {
+    runHttpRemoteTests = TestUtilities.getPropValueInt("testng.runHttpRemoteTests") > 0;
+    runS3RemoteTests = TestUtilities.getPropValueInt("testng.runS3RemoteTests") > 0;
 
-    if (!isOnline) {
-      System.err.println("WARNING: online tests are disabled!");
+    if (!runHttpRemoteTests) {
+      System.err.println("WARNING: HTTP tests are disabled!");
     }
-    if (!canAccessS3) {
+    if (!runS3RemoteTests) {
       System.err.println("WARNING: S3 tests are disabled!");
     }
   }
 
-  private void skipIfOffline(int i) throws SkipException {
-    if (isRemote[i] == LocalRemoteType.HTTP && !isOnline) {
-      throw new SkipException("must be online to test " + files[i].getName());
+  private void skipIfHttpDisabled(int i) throws SkipException {
+    if (isRemote[i] == LocalRemoteType.HTTP && !runHttpRemoteTests) {
+      throw new SkipException("HTTP tests are disabled " + files[i].getName());
     }
   }
 
-  private void skipIfS3Offline(int i) throws SkipException {
-    if (isRemote[i] == LocalRemoteType.S3 && !canAccessS3) {
-      throw new SkipException("must have access to s3 to test " + files[i].getName());
+  private void skipIfS3Disabled(int i) throws SkipException {
+    if (isRemote[i] == LocalRemoteType.S3 && !runS3RemoteTests) {
+      throw new SkipException("S3 tests are disabled " + files[i].getName());
     }
   }
 
@@ -238,8 +227,8 @@ private void skipIfS3Offline(int i) throws SkipException {
   @Test
   public void testReadWriteMode() {
     for (int i=0; i<files.length; i++) {
-      skipIfOffline(i);
-      skipIfS3Offline(i);
+      skipIfHttpDisabled(i);
+      skipIfS3Disabled(i);
       String msg = files[i].getName();
       assertEquals(msg, mode[i].contains("r"), files[i].canRead());
       assertEquals(msg, mode[i].contains("w"), files[i].canWrite());
@@ -256,8 +245,8 @@ public void testAbsolute() {
   @Test
   public void testExists() {
     for (int i=0; i<files.length; i++) {
-      skipIfOffline(i);
-      skipIfS3Offline(i);
+      skipIfHttpDisabled(i);
+      skipIfS3Disabled(i);
       assertEquals(files[i].getName(), exists[i], files[i].exists());
     }
   }
@@ -286,7 +275,7 @@ public void testParentRoot() {
   @Test
   public void testIsDirectory() {
     for (int i=0; i<files.length; i++) {
-      skipIfS3Offline(i);
+      skipIfS3Disabled(i);
       assertEquals(files[i].getName(), isDirectory[i], files[i].isDirectory());
     }
   }
@@ -294,8 +283,8 @@ public void testIsDirectory() {
   @Test
   public void testIsFile() {
     for (int i=0; i<files.length; i++) {
-      skipIfOffline(i);
-      skipIfS3Offline(i);
+      skipIfHttpDisabled(i);
+      skipIfS3Disabled(i);
       assertEquals(files[i].getName(), !isDirectory[i] && exists[i], files[i].isFile());
     }
   }
diff --git a/src/test/java/loci/common/utests/TestUtilities.java b/src/test/java/loci/common/utests/TestUtilities.java
new file mode 100644
index 00000000..7d37ddc4
--- /dev/null
+++ b/src/test/java/loci/common/utests/TestUtilities.java
@@ -0,0 +1,51 @@
+/*
+ * #%L
+ * BSD implementations of Bio-Formats readers and writers
+ * %%
+ * Copyright (C) 2005 - 2018 Open Microscopy Environment:
+ *   - Board of Regents of the University of Wisconsin-Madison
+ *   - Glencoe Software, Inc.
+ *   - University of Dundee
+ * %%
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * 
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ * 
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ * #L%
+ */
+package loci.common.utests;
+
+import loci.common.DataTools;
+
+public final class TestUtilities {
+
+  /**
+   * Lookup an integer property
+   * @param propertyName Lookup this property
+   * @return an integer, 0 if not found
+   */
+  public static int getPropValueInt(String propertyName) {
+    String prop = System.getProperty(propertyName);
+    if (prop == null ||
+        prop.equals("${"+ propertyName + "}")) return 0;
+    if (DataTools.parseInteger(prop) == null) return 0;
+    int propertyValue = DataTools.parseInteger(prop);
+    return propertyValue;
+  }
+}

From 34866f5b9a548ff7572cd0956c8b287214f75ef7 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 1 Nov 2018 17:23:14 +0000
Subject: [PATCH 42/55] Remove skip(-1) in S3Handle.resetStream

It's not clear why it's required, and it's possible it may cause problems if the DataInputStream supports reverse seek in future.
---
 src/main/java/loci/common/S3Handle.java | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 78d558a4..51c92488 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -427,7 +427,6 @@ protected void resetStream(long offset) throws IOException {
       length = stat.length();
       stream = new DataInputStream(new BufferedInputStream(
               s3Client.getObject(bucket, path, offset)));
-      stream.skip(-1L);
       fp = offset;
       mark = offset;
       }

From 045b5d855f3da65ee9cbd7f08b67387e965a951b Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 2 Nov 2018 10:54:32 +0000
Subject: [PATCH 43/55] Remove unused imports, avoid wildcard imports

---
 src/main/java/loci/common/Location.java | 7 +++++--
 src/main/java/loci/common/S3Handle.java | 3 ---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 6b3d4473..dd53abba 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -38,13 +38,16 @@
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.UncheckedIOException;
-import java.net.*;
+import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.net.URLConnection;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import org.slf4j.Logger;
diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 51c92488..29a08a22 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -35,7 +35,6 @@
 import java.io.BufferedInputStream;
 import java.io.DataInputStream;
 import java.io.IOException;
-import java.net.ConnectException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.nio.file.Files;
@@ -43,13 +42,11 @@
 import java.nio.file.Paths;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import io.minio.MinioClient;
 import io.minio.errors.MinioException;
 import io.minio.ObjectStat;
-import io.minio.errors.*;
 import org.xmlpull.v1.XmlPullParserException;
 
 import org.slf4j.Logger;

From 8f7771ee057b10da036fb0ecc3e2a99de53e4145 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 2 Nov 2018 13:00:34 +0000
Subject: [PATCH 44/55] Don't catch IOException in S3Handle constructor

If an exception is caught then output DEBUG message
---
 src/main/java/loci/common/S3Handle.java | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 29a08a22..23527d13 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -213,20 +213,19 @@ else if (scheme.startsWith("s3+")) {
     this.stat = null;
 
     if (initialize) {
-      // Throw if there is a connection error, otherwise save the exception and only throw if a method
+      // Throw if there is an IOException, otherwise save the exception and only throw if a method
       // that requires a valid object is called
       this.connect();
       try {
         this.initialize();
       }
       catch (
-        IOException |
         MinioException |
         InvalidKeyException |
         NoSuchAlgorithmException |
         XmlPullParserException e) {
         this.objectNotFound = e;
-        LOGGER.trace("Object not found: {}", this);
+        LOGGER.debug("Object not found: {}", this);
       }
       LOGGER.trace("isBucket:{} stat:{}", isBucket, stat);
     }

From 4b5a08244c452a2b3fee8a4701f9c572fc024dd4 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 2 Nov 2018 14:05:08 +0000
Subject: [PATCH 45/55] Add exists() to IRandomAccess

---
 .../java/loci/common/AbstractNIOHandle.java     |  6 ++++++
 src/main/java/loci/common/FileHandle.java       |  6 ++++++
 src/main/java/loci/common/IRandomAccess.java    |  8 ++++++++
 src/main/java/loci/common/StreamHandle.java     | 17 ++++++-----------
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/main/java/loci/common/AbstractNIOHandle.java b/src/main/java/loci/common/AbstractNIOHandle.java
index f614d25e..db9e13f3 100644
--- a/src/main/java/loci/common/AbstractNIOHandle.java
+++ b/src/main/java/loci/common/AbstractNIOHandle.java
@@ -56,6 +56,12 @@ public abstract class AbstractNIOHandle implements IRandomAccess {
 
   // -- AbstractNIOHandle methods --
 
+  /* @see IRandomAccess#exists() */
+  @Override
+  public boolean exists() throws IOException {
+    return length() >= 0;
+  }
+
   /**
    * Ensures that the file mode is either "r" or "rw".
    * @param mode Mode to validate.
diff --git a/src/main/java/loci/common/FileHandle.java b/src/main/java/loci/common/FileHandle.java
index c8bddd79..2203c4e9 100644
--- a/src/main/java/loci/common/FileHandle.java
+++ b/src/main/java/loci/common/FileHandle.java
@@ -101,6 +101,12 @@ public long getFilePointer() throws IOException {
     return raf.getFilePointer();
   }
 
+  /* @see IRandomAccess#exists() */
+  @Override
+  public boolean exists() throws IOException {
+    return length() >= 0;
+  }
+
   /* @see IRandomAccess.length() */
   @Override
   public long length() throws IOException {
diff --git a/src/main/java/loci/common/IRandomAccess.java b/src/main/java/loci/common/IRandomAccess.java
index f4acc887..bc4b8446 100644
--- a/src/main/java/loci/common/IRandomAccess.java
+++ b/src/main/java/loci/common/IRandomAccess.java
@@ -61,6 +61,14 @@ public interface IRandomAccess extends DataInput, DataOutput {
    */
   long getFilePointer() throws IOException;
 
+  /**
+   * Returns whether this refers to a valid object
+   *
+   * @return true if this refers to a valid object
+   * @throws IOException if unable to determine whether the object is valid
+   */
+  boolean exists() throws IOException;
+
   /**
    * Returns the length of this stream.
    *
diff --git a/src/main/java/loci/common/StreamHandle.java b/src/main/java/loci/common/StreamHandle.java
index a3f785f5..dc045c94 100644
--- a/src/main/java/loci/common/StreamHandle.java
+++ b/src/main/java/loci/common/StreamHandle.java
@@ -120,6 +120,12 @@ public long getFilePointer() throws IOException {
     return fp;
   }
 
+  /* @see IRandomAccess#exists() */
+  @Override
+  public boolean exists() throws IOException {
+    return length >= 0;
+  }
+
   /* @see IRandomAccess#length() */
   @Override
   public long length() throws IOException {
@@ -478,17 +484,6 @@ public void writeUTF(String str) throws IOException {
     outStream.writeUTF(str);
   }
 
-  // -- Public methods --
-
-  /**
-   * Does this represent an accessible location?
-   * @return true if this location is accessible
-   * @throws IOException if unable to determine whether this location is accessible
-   */
-  public boolean exists() throws IOException {
-    return length >= 0;
-  }
-
   // -- Helper methods --
 
   /**

From 76bea964a24162ed6e239c08dbed503356b2ace1 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Fri, 2 Nov 2018 16:36:17 +0000
Subject: [PATCH 46/55] Bump to minio 5.0.2

---
 pom.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pom.xml b/pom.xml
index 64892afb..d29419a3 100644
--- a/pom.xml
+++ b/pom.xml
@@ -104,7 +104,7 @@
     <dependency>
      <groupId>io.minio</groupId>
      <artifactId>minio</artifactId>
-     <version>5.0.0</version>
+     <version>5.0.2</version>
     </dependency>
     <dependency>
      <groupId>com.esotericsoftware.kryo</groupId>

From 7d7c4806d54e835aced62f5f999b9e77586a0cf0 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Mon, 5 Nov 2018 11:27:46 +0000
Subject: [PATCH 47/55] skip more LocationTests if S3

---
 src/test/java/loci/common/utests/LocationTest.java | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/test/java/loci/common/utests/LocationTest.java b/src/test/java/loci/common/utests/LocationTest.java
index 11a1eab7..dc6c5371 100644
--- a/src/test/java/loci/common/utests/LocationTest.java
+++ b/src/test/java/loci/common/utests/LocationTest.java
@@ -299,6 +299,7 @@ public void testIsHidden() {
   @Test
   public void testListFiles() {
     for (int i=0; i<files.length; i++) {
+      skipIfS3Disabled(i);
       String[] completeList = files[i].list();
       String[] unhiddenList = files[i].list(true);
       Location[] fileList = files[i].listFiles();
@@ -326,7 +327,10 @@ public void testListFiles() {
 
   @Test
   public void testToURL() throws IOException {
-    for (Location file : files) {
+    for (int i=0; i<files.length; i++) {
+      // S3 isDirectory will throw if connection fails
+      skipIfS3Disabled(i);
+      Location file = files[i];
       String path = file.getAbsolutePath();
       if (!path.contains("://")) {
         if (IS_WINDOWS) {

From cb233873894f458557472ba3a2cf05e7637a73f7 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 6 Nov 2018 16:38:05 +0000
Subject: [PATCH 48/55] Add more info when object not found exception throw

Include the underlying Minio exception if these was one
---
 src/main/java/loci/common/S3Handle.java | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index 23527d13..d71e62f8 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -63,6 +63,15 @@
  */
 public class S3Handle extends StreamHandle {
 
+  /**
+   * An S3 IOException that was not thrown immediately
+   */
+  class DelayedObjectNotFound extends IOException {
+    DelayedObjectNotFound(S3Handle s3) {
+      super(String.format("Object not found: [%s] %s", s3, s3.objectNotFound), s3.objectNotFound);
+    }
+  }
+
   /** Default protocol for fetching s3:// */
   public final static String DEFAULT_S3_PROTOCOL = "https";
 
@@ -225,7 +234,7 @@ else if (scheme.startsWith("s3+")) {
         NoSuchAlgorithmException |
         XmlPullParserException e) {
         this.objectNotFound = e;
-        LOGGER.debug("Object not found: {}", this);
+        LOGGER.debug("Object not found: [{}] {}", this, e);
       }
       LOGGER.trace("isBucket:{} stat:{}", isBucket, stat);
     }
@@ -358,7 +367,7 @@ protected void downloadObject(Path destination) throws HandleException, IOExcept
    */
   public boolean isBucket() {
     //if (this.objectNotFound != null) {
-    //  throw new IOException("Object not found " + this, this.objectNotFound);
+    //  throw new DelayedObjectNotFound(this);
     //}
     return isBucket;
   }
@@ -367,7 +376,7 @@ public boolean isBucket() {
   @Override
   public long length() throws IOException {
     if (this.stat == null || this.objectNotFound != null) {
-      throw new IOException("Object not found " + this, this.objectNotFound);
+      throw new DelayedObjectNotFound(this);
     }
     return length;
   }
@@ -379,7 +388,7 @@ public long length() throws IOException {
   public void seek(long pos) throws IOException {
     LOGGER.trace("{}", pos);
     if (this.stat == null || this.objectNotFound != null) {
-      throw new IOException("Object not found " + this, this.objectNotFound);
+      throw new DelayedObjectNotFound(this);
     }
     long diff = pos - fp;
 
@@ -417,7 +426,7 @@ public boolean exists() throws IOException {
   protected void resetStream(long offset) throws IOException {
     LOGGER.trace("Resetting {}", offset);
     if (this.stat == null || this.objectNotFound != null) {
-      throw new IOException("Object not found " + this, this.objectNotFound);
+      throw new DelayedObjectNotFound(this);
     }
     try {
       length = stat.length();

From fe7b76c0c8ae7474ebdd1b9b2c6586d4552f6c41 Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 6 Nov 2018 17:10:14 +0000
Subject: [PATCH 49/55] Don't show found status in connect trace message

It's misleading since we don't know whether it's found or not this early
---
 src/main/java/loci/common/S3Handle.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index d71e62f8..cfac2e6f 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -252,7 +252,7 @@ protected void connect() throws IOException {
       throw new IOException(String.format(
               "Failed to connect: %s", this), e);
     }
-    LOGGER.trace("connected:{}", this);
+    LOGGER.trace("connected: server:{} port:{}", server, port);
   }
 
   /**

From a5062121008f40761f629c5014e43165a45717fc Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Wed, 7 Nov 2018 17:34:01 +0000
Subject: [PATCH 50/55] Location.exists don't cast handle

---
 src/main/java/loci/common/Location.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index dd53abba..7f605aa3 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -725,7 +725,7 @@ public boolean exists() {
     if (isURL) {
       try {
         // TODO: existence should almost certainly be cached.
-        StreamHandle handle = (StreamHandle) getHandle(uri.toString());
+        IRandomAccess handle = getHandle(uri.toString());
         boolean exists = handle.exists();
         handle.close();
         return exists;

From 740a88b6c5030e9e9e46ac2a97d9dd79c25f6fac Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 8 Nov 2018 11:49:50 +0000
Subject: [PATCH 51/55] S3Handle set user-agent to `Bio-Formats/dev`

---
 src/main/java/loci/common/S3Handle.java | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/main/java/loci/common/S3Handle.java b/src/main/java/loci/common/S3Handle.java
index cfac2e6f..9862007e 100644
--- a/src/main/java/loci/common/S3Handle.java
+++ b/src/main/java/loci/common/S3Handle.java
@@ -247,6 +247,8 @@ else if (scheme.startsWith("s3+")) {
   protected void connect() throws IOException {
     try {
       s3Client = new MinioClient(server, port, accessKey, secretKey);
+      // TODO: Replace "dev" with a version
+      s3Client.setAppInfo("Bio-Formats", "dev");
     }
     catch (MinioException e) {
       throw new IOException(String.format(

From 6811bacec4a0cb90f83da6489d4f14b004141b3c Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 13 Nov 2018 15:02:36 +0000
Subject: [PATCH 52/55] Create 2MB file for testing

---
 location-server/start-location.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/location-server/start-location.sh b/location-server/start-location.sh
index 040abf84..cb9f9343 100755
--- a/location-server/start-location.sh
+++ b/location-server/start-location.sh
@@ -18,12 +18,17 @@ sleep 2;
 
 ./mc config host add ome-common-java-minio-test http://localhost:$PORT ${MINIO_ACCESS_KEY} ${MINIO_SECRET_KEY}
 
+set +x
+
 for SUFFIX in public private; do
     ./mc ls ome-common-java-minio-test/bioformats.test.$SUFFIX || \
         ./mc mb ome-common-java-minio-test/bioformats.test.$SUFFIX
     ./mc ls ome-common-java-minio-test/bioformats.test.$SUFFIX/single-channel.ome.tiff || \
         curl -sfS https://downloads.openmicroscopy.org/images/OME-TIFF/2016-06/bioformats-artificial/single-channel.ome.tiff | \
             ./mc pipe ome-common-java-minio-test/bioformats.test.$SUFFIX/single-channel.ome.tiff
+    # 2MB file for testing seek
+    for n in `seq 65536`; do printf '.% 30d\n' $n; done | \
+        ./mc pipe ome-common-java-minio-test/bioformats.test.$SUFFIX/2MBfile.txt
 done
 
 ./mc policy public ome-common-java-minio-test/bioformats.test.public

From 37209818116d0b0cc6a5b570b61430b0379c2eda Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Tue, 13 Nov 2018 15:36:49 +0000
Subject: [PATCH 53/55] Test S3Handle read and seek

---
 .../java/loci/common/utests/S3HandleTest.java | 150 +++++++++++++++++-
 1 file changed, 145 insertions(+), 5 deletions(-)

diff --git a/src/test/java/loci/common/utests/S3HandleTest.java b/src/test/java/loci/common/utests/S3HandleTest.java
index 405b7763..8b86428e 100644
--- a/src/test/java/loci/common/utests/S3HandleTest.java
+++ b/src/test/java/loci/common/utests/S3HandleTest.java
@@ -34,10 +34,14 @@
 
 import loci.common.S3Handle;
 import loci.common.StreamHandle;
-import org.testng.annotations.BeforeMethod;
+import org.testng.SkipException;
+import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 
 import static org.testng.AssertJUnit.assertEquals;
 import static org.testng.AssertJUnit.assertFalse;
@@ -53,11 +57,30 @@ public class S3HandleTest {
 
   // -- Fields --
 
+  private static boolean runS3RemoteTests;
+  private Path TEMPDIR;
+
+  private static final String s3public = "s3+http://localhost:31836";
+  private static final String s3private = "s3+http://accesskey:secretkey@localhost:31836";
+
   // -- Setup methods --
 
-  @BeforeMethod
-  public void setup() {
-    // no-op
+  @BeforeClass
+  public void setup() throws IOException {
+    TEMPDIR = Files.createTempDirectory("S3HandleTest-");
+    TEMPDIR.toFile().deleteOnExit();
+
+    runS3RemoteTests = TestUtilities.getPropValueInt("testng.runS3RemoteTests") > 0;
+
+    if (!runS3RemoteTests) {
+      System.err.println("WARNING: S3 tests are disabled!");
+    }
+  }
+
+  private void skipIfS3Disabled() throws SkipException {
+    if (!runS3RemoteTests) {
+      throw new SkipException("S3 tests are disabled");
+    }
   }
 
   // -- Test methods --
@@ -154,4 +177,121 @@ public void testParseBucketSlash() throws IOException {
     assertEquals("bucket", s3.getBucket());
     assertEquals(null, s3.getPath());
   }
-}
+
+  @Test
+  public void testIsBucket() throws IOException {
+    skipIfS3Disabled();
+    S3Handle s3 = new S3Handle(s3public + "/bioformats.test.public");
+    assertTrue(s3.isBucket());
+  }
+
+  @Test
+  public void testReadPublic() throws IOException {
+    skipIfS3Disabled();
+    S3Handle s3 = new S3Handle(s3public + "/bioformats.test.public/single-channel.ome.tiff");
+    assertFalse(s3.isBucket());
+    assertTrue(s3.exists());
+    assertEquals(76097, s3.length());
+  }
+
+  @Test
+  public void testReadPrivate() throws IOException {
+    skipIfS3Disabled();
+    S3Handle s3 = new S3Handle(s3private + "/bioformats.test.private/single-channel.ome.tiff");
+    assertFalse(s3.isBucket());
+    assertTrue(s3.exists());
+    assertEquals(76097, s3.length());
+  }
+
+  @Test
+  public void testReadAndSeek() throws IOException {
+    skipIfS3Disabled();
+    S3Handle s3 = new S3Handle(s3public + "/bioformats.test.public/2MBfile.txt");
+    assertFalse(s3.isBucket());
+    assertTrue(s3.exists());
+    assertEquals(2097152, s3.length());
+
+    byte[] buffer = new byte[32];
+    int r;
+
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals(".                             1\n", new String(buffer));
+
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals(".                             2\n", new String(buffer));
+
+    s3.seek(80);
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals("              3\n.               ", new String(buffer));
+
+    // Large seek (S3Handle.S3_MAX_FORWARD_SEEK)
+    s3.seek(2097056);
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals(".                         65534\n", new String(buffer));
+
+    // Reverse seek
+    s3.seek(144);
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals("              5\n.               ", new String(buffer));
+  }
+
+  @Test
+  public void testResetStream() throws IOException {
+    class S3HandleWrapper extends S3Handle {
+      public S3HandleWrapper(String url) throws IOException {
+        super(url);
+      }
+
+      @Override
+      public void resetStream() throws IOException {
+        super.resetStream();
+      }
+
+      @Override
+      public void resetStream(long offset) throws IOException {
+        super.resetStream(offset);
+      }
+    }
+
+    skipIfS3Disabled();
+    S3HandleWrapper s3 = new S3HandleWrapper(s3private + "/bioformats.test.private/2MBfile.txt");
+    assertFalse(s3.isBucket());
+    assertTrue(s3.exists());
+    assertEquals(2097152, s3.length());
+
+    byte[] buffer = new byte[32];
+    int r;
+    s3.resetStream(750144);
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals(".                         23443\n", new String(buffer));
+
+    s3.resetStream();
+    r = s3.read(buffer, 0, 32);
+    assertEquals(32, r);
+    assertEquals(".                             1\n", new String(buffer));
+  }
+
+  @Test
+  public void testCache() throws IOException {
+    class MockSettings extends StreamHandle.Settings {
+      @Override
+      public String getRemoteCacheRootDir() {
+        return TEMPDIR.toString();
+      }
+    }
+
+    skipIfS3Disabled();
+    final String expectedPath = TEMPDIR + "/http/localhost/31836/bioformats.test.public/2MBfile.txt";
+
+    String downloaded = S3Handle.cacheObject(
+        s3public + "/bioformats.test.public/2MBfile.txt", new MockSettings());
+    assertEquals(expectedPath, downloaded);
+    assertEquals(2097152, Files.size(Paths.get(downloaded)));
+  }
+}
\ No newline at end of file

From 795cc2b435c997160fcb91df59c4150e1152663e Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 15 Nov 2018 14:43:51 +0000
Subject: [PATCH 54/55] Add note about handle caching (doesn't work)

---
 src/main/java/loci/common/Location.java | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 7f605aa3..33c400e9 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -485,6 +485,9 @@ else if (allowArchiveHandles && BZip2Handle.isBZip2File(mapId)) {
         }
       }
       LOGGER.trace("Created new handle {} -> {}", id, handle);
+      // TODO: We should cache the handle, but we can't prevent callers from closing it which
+      // would make the cached handle useless to future fetches
+      //mapFile(id, handle);
     }
     LOGGER.trace("Location.getHandle: {} -> {}", id, handle);
     return handle;

From 056b1cb736d71a75db94298738b5206d8721779e Mon Sep 17 00:00:00 2001
From: Simon Li <spli@dundee.ac.uk>
Date: Thu, 15 Nov 2018 18:22:17 +0000
Subject: [PATCH 55/55] Cache URL exists and length

---
 src/main/java/loci/common/Location.java | 70 +++++++++++++++++--------
 1 file changed, 48 insertions(+), 22 deletions(-)

diff --git a/src/main/java/loci/common/Location.java b/src/main/java/loci/common/Location.java
index 33c400e9..a5fc14e2 100644
--- a/src/main/java/loci/common/Location.java
+++ b/src/main/java/loci/common/Location.java
@@ -118,6 +118,44 @@ protected class ListingsResult {
   private URI uri;
   private File file;
 
+  class URLLocationProperties {
+    public final long length;
+    public final boolean exists;
+
+    public URLLocationProperties(Location loc) {
+      LOGGER.trace("Getting LocationProperties");
+      boolean bexists = false;
+      long llength = 0;
+      if (!loc.isURL) {
+        throw new IllegalArgumentException("Location must be a URL");
+      }
+      try {
+        IRandomAccess handle = Location.getHandle(uri.toString());
+        try {
+          bexists = handle.exists();
+        }
+        catch (IOException e) {
+          LOGGER.trace("Failed to retrieve content from URL", e);
+        }
+        if (bexists) {
+          try {
+            llength = handle.length();
+          } catch (IOException e) {
+            LOGGER.trace("Could not determine URL's content length", e);
+          }
+        }
+        handle.close();
+      }
+      catch (IOException e) {
+        LOGGER.trace("Failed to retrieve content from URL", e);
+      }
+      this.exists = bexists;
+      this.length = llength;
+      LOGGER.trace("exists:{} length:{}", bexists, llength);
+    }
+  }
+  private URLLocationProperties cachedProperties;
+
   // -- Constructors --
 
   /**
@@ -724,20 +762,14 @@ public int hashCode() {
    * @see java.io.File#exists()
    */
   public boolean exists() {
-    LOGGER.trace("exists()");
     if (isURL) {
-      try {
-        // TODO: existence should almost certainly be cached.
-        IRandomAccess handle = getHandle(uri.toString());
-        boolean exists = handle.exists();
-        handle.close();
-        return exists;
-      }
-      catch (IOException e) {
-        LOGGER.trace("Failed to retrieve content from URL", e);
-        return false;
+      LOGGER.trace("exists(url)");
+      if (cachedProperties == null) {
+        cachedProperties = new URLLocationProperties(this);
       }
+      return cachedProperties.exists;
     }
+    LOGGER.trace("exists(file)");
     if (file.exists()) return true;
     if (getMappedFile(file.getPath()) != null) return true;
 
@@ -966,19 +998,13 @@ public long lastModified() {
    * @see java.net.URLConnection#getContentLength()
    */
   public long length() {
-    LOGGER.trace("length()");
     if (isURL) {
-      try {
-        IRandomAccess handle = getHandle(uri.toString());
-        long len = handle.length();
-        handle.close();
-        return len;
-      }
-      catch (IOException e) {
-        LOGGER.trace("Could not determine URL's content length", e);
-        return 0;
-      }
+      LOGGER.trace("length(url)");
+      // Ensure cachedProperties is populated
+      exists();
+      return cachedProperties.length;
     }
+    LOGGER.trace("length(file)");
     return file.length();
   }