Merge pull request #152 from saalfeldlab/fix/uriResolveJava8

Fix/uri resolve java8

Author: bogovicj

src/main/java/org/janelia/saalfeldlab/n5/FileSystemKeyValueAccess.java

@@ -325,6 +325,8 @@ public String compose(final String... components) {
 		else
 			composedPath = Paths.get(uri.toString());
 		for (String component : components) {
+			if (component == null || component.isEmpty())
+				continue;
 			composedPath = composedPath.resolve(component);
 		}
 

src/main/java/org/janelia/saalfeldlab/n5/KeyValueAccess.java

@@ -80,13 +80,28 @@ public default String[] components(final String path) {
 	 */
 	public default String compose(final URI uri, final String... components) {
 
-		if (components.length == 0)
+		int firstNonEmptyIdx = 0;
+		while (firstNonEmptyIdx < components.length && (components[firstNonEmptyIdx] == null || components[firstNonEmptyIdx].isEmpty())) {
+			firstNonEmptyIdx++;
+		}
+
+		/*If there are no non-empty components, there is nothing to compose against; return the uri. */
+		if (components.length == firstNonEmptyIdx)
 			return uri.toString();
 
-		/* add the initial path to the components */
-		final String[] allComponents = new String[components.length + 1];
-		allComponents[0] = uri.getPath();
-		System.arraycopy(components, 0, allComponents, 1, components.length);
+		/* allocate space for the initial path and the new components, skipping empty strings  */
+		final int nonEmptysize = components.length - firstNonEmptyIdx;
+		final String[] allComponents = new String[1 + nonEmptysize];
+		if (uri.getPath().isEmpty())
+			//TODO Caleb: This `isEmpty()` check is only necessary for Java 8. In newer versions
+			//	URI resolution is updated so that resolving and empty path with a new path adds
+			//	a leading `/` between the rest of the URI and the path part. In Java 8 it doesn't
+			//  add the `/` so it ends up directly concatenating the path part with URI
+			allComponents[0] = "/";
+		else
+			allComponents[0] = uri.getPath();
+
+		System.arraycopy(components, firstNonEmptyIdx, allComponents, 1, nonEmptysize);
 
 		URI composedUri = uri;
 		for (int i = 0; i < allComponents.length; i++) {

src/test/java/org/janelia/saalfeldlab/n5/AbstractN5Test.java

@@ -45,6 +45,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Random;
+import java.util.UUID;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 import java.util.function.Predicate;
@@ -1074,12 +1075,16 @@ public void testVersion() throws NumberFormatException, IOException, URISyntaxEx
 	@Test
 	public void testReaderCreation() throws IOException, URISyntaxException {
 
-		final String location;
-		N5Writer removeMe = null;
-		try (N5Writer writer = createN5Writer()) {
-			removeMe = writer;
-			location = writer.getURI().toString();
+		// non-existent location should fail
+		final String location = tempN5Location() + "-" + UUID.randomUUID();
+		assertThrows("Non-existent location throws error", N5Exception.N5IOException.class,
+				() -> {
+					try (N5Reader test = createN5Reader(location)) {
+						test.list("/");
+					}
+				});
 
+		try (N5Writer writer = createTempN5Writer(location)) {
 			try (N5Reader n5r = createN5Reader(location)) {
 				assertNotNull(n5r);
 			}
@@ -1107,17 +1112,7 @@ public void testReaderCreation() throws IOException, URISyntaxException {
 					 /*Only try with resource to ensure `close()` is called.*/
 				}
 			});
-		} finally {
-			removeMe.remove();
 		}
-
-		// non-existent location should fail
-		assertThrows("Non-existent location throws error", N5Exception.N5IOException.class,
-				() -> {
-					try (N5Reader test = createN5Reader(location)) {
-						test.list("/");
-					}
-				});
 	}
 
 	@Test

src/test/java/org/janelia/saalfeldlab/n5/N5CachedFSTest.java

@@ -15,6 +15,7 @@
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -422,8 +423,8 @@ public static void cacheBehaviorHelper(final TrackingStorage n5) throws IOExcept
 		assertEquals(++expectedAttributeCount, n5.getAttrCallCount());
 		assertEquals(expectedWriteAttributeCount, n5.getWriteAttrCallCount());
 
-		final Set<String> listSet = Arrays.stream(n5.list("a")).collect(Collectors.toSet());
-		assertEquals(Stream.of("a", "b", "c").collect(Collectors.toSet()), listSet);
+		final Set<String> abcListSet = Arrays.stream(n5.list("a")).collect(Collectors.toSet());
+		assertEquals(Stream.of("a", "b", "c").collect(Collectors.toSet()), abcListSet);
 		assertEquals(expectedGroupCount, n5.getGroupCallCount());
 		assertEquals(expectedDatasetCount, n5.getDatasetCallCount());
 		assertEquals(expectedAttributeCount, n5.getAttrCallCount());
@@ -432,7 +433,8 @@ public static void cacheBehaviorHelper(final TrackingStorage n5) throws IOExcept
 
 		// remove a
 		n5.remove("a/a");
-		assertArrayEquals(new String[] {"b", "c"}, n5.list("a")); // call list
+		final Set<String> bc = Arrays.stream(n5.list("a")).collect(Collectors.toSet());
+		assertEquals(Stream.of("b", "c").collect(Collectors.toSet()), bc);
 		assertEquals(expectedExistCount, n5.getExistCallCount());
 		assertEquals(expectedGroupCount, n5.getGroupCallCount());
 		assertEquals(expectedDatasetCount, n5.getDatasetCallCount());

src/test/java/org/janelia/saalfeldlab/n5/kva/AbstractKeyValueAccessTest.java

@@ -8,6 +8,8 @@
 import org.junit.Test;
 
 import java.net.URI;
+import java.util.LinkedHashSet;
+import java.util.Set;
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
@@ -18,31 +20,43 @@
 public abstract class AbstractKeyValueAccessTest {
 
 	protected abstract KeyValueAccess newKeyValueAccess(URI root);
+
 	protected KeyValueAccess newKeyValueAccess() {
+
 		return newKeyValueAccess(tempUri());
 	}
 
 	protected abstract URI tempUri();
 
 	protected URI[] testURIs(final URI base) {
+
+		final Set<URI> testUris = new LinkedHashSet<>();
+		/*add the base uri as a test case */
+		testUris.add(base);
+
+		/* NOTE: Java 8 doesn't behave well with URIs with empty path when resolving against a path.
+		*   See KeyValueAccess#compose for more details.
+		* 	In tests with that as a base URI, resolve against `/` first.
+		* 	Should be unnecessary in Java 21*/
+		final URI testUri = base.getPath().isEmpty() ? base.resolve("/") : base;
 		final URI[] pathParts = new URI[]{
-				N5URI.getAsUri("test/path/file"),	 // typical path, with leading and trailing slash
-				N5URI.getAsUri("test/path/file/"),	 // typical path, with leading and trailing slash
-				N5URI.getAsUri("/test/path/file/"),	 // typical path, with leading and trailing slash
-				N5URI.getAsUri("file"),  			 // single path
-				N5URI.getAsUri("file/"),			 // single path
-				N5URI.getAsUri("/file/"),			 // single path
+				N5URI.getAsUri("test/path/file"),     // typical path, with no leading or trailing slashes
+				N5URI.getAsUri("test/path/file/"),     // typical path, with trailing slash
+				N5URI.getAsUri("/test/path/file"),     // typical path, with leading slash
+				N5URI.getAsUri("/test/path/file/"),     // typical path, with leading and trailing slash
+				N5URI.getAsUri("file"),             // single path
+				N5URI.getAsUri("file/"),             // single path
+				N5URI.getAsUri("/file"),             // single path
+				N5URI.getAsUri("/file/"),             // single path
 				N5URI.getAsUri("path/w i t h/spaces"),
 				N5URI.getAsUri("uri/illegal%character"),
-				N5URI.getAsUri("/"),
-				N5URI.getAsUri("")
+				N5URI.getAsUri("/"), 				// root path
+				N5URI.getAsUri("")					// empty path
 		};
-		final URI[] testUris = new URI[pathParts.length ];
-		for (int i = 0; i < pathParts.length; i ++) {
-			final URI pathPart = pathParts[i];
-			testUris[i] = base.resolve(pathPart);
+		for (final URI pathPart : pathParts) {
+			testUris.add(testUri.resolve(pathPart));
 		}
-		return testUris;
+		return testUris.toArray(new URI[0]);
 	}
 
 	protected String[][] testPathComponents(final URI base) {
@@ -81,7 +95,7 @@ protected void testComponentsAtLocation(URI testRoot) {
 
 			final String[] components = access.components(testPaths[i].getPath());
 
-			assertArrayEquals("Failure at Index " + i ,expectedComponents[i], components);
+			assertArrayEquals("Failure at Index " + i, expectedComponents[i], components);
 		}
 	}
 
@@ -96,11 +110,12 @@ protected void testComposeAtLocation(URI uri) {
 
 		for (int i = 0; i < testPathComponents.length; ++i) {
 			testPathComponents[i] = testPathComponents[i].clone();
-			final URI baseUri = testUris[i].resolve("/");
+			/* Don't add the "/" if the input uri path is empty. just use it. Otherwise, remove the parts and start with "/" */
+			final URI baseUri = uri.getPath().isEmpty() ? uri : testUris[i].resolve("/");
 			final String[] components = testPathComponents[i];
-			final String stringUriFromComponents = access.compose(baseUri, components);
-			final URI uriFromComponents = N5URI.getAsUri(stringUriFromComponents);
-			assertEquals("Failure at Index " + i , testUris[i], uriFromComponents);
+			final String actualCompose = access.compose(baseUri, components);
+			final String expectedCompose = access.compose(testUris[i]);
+			assertEquals("Failure at Index " + i, expectedCompose, actualCompose);
 		}
 	}
 
@@ -111,21 +126,83 @@ public void testComponents() {
 	}
 
 	@Test
-	public void testComponentsAtRoot() {
+	public void testComponentsWithPathSlash() {
+
+		final URI uriWithPathSlash = setUriPath(tempUri(), "/");
+		testComponentsAtLocation(uriWithPathSlash);
+	}
+
+	@Test
+	public void testComponentsWithPathEmpty() {
 
-		URI root = tempUri().resolve("/");
-		testComponentsAtLocation(root);
+		final URI uriWithPathEmpty = setUriPath(tempUri(), "");
+		testComponentsAtLocation(uriWithPathEmpty);
 	}
 
 	@Test
 	public void testCompose() {
-		testComposeAtLocation(tempUri());
+
+		final URI uri = tempUri();
+		testComposeAtLocation(uri);
+
+		final KeyValueAccess kva = newKeyValueAccess();
+		final URI uriWithPath = setUriPath(uri, "/foo");
+		assertEquals("Non-empty Path", "/foo", uriWithPath.getPath());
+		assertComposeEquals("Non-empty Path, no empty or slash in components", kva, uriWithPath, "/foo/bar/baz", "bar", "baz");
+        assertComposeEquals("Non-empty Path, first components leading slash", kva, uriWithPath, "/bar/baz", "/bar", "baz");
+        assertComposeEquals("Non-empty Path, first components slash only", kva, uriWithPath,"/bar/baz", "/", "bar", "baz");
+        assertComposeEquals("Non-empty Path, first components slash only", kva, uriWithPath,"/foo/bar/baz", "", "bar", "baz");
+        assertComposeEquals("Non-empty Path, first components slash only", kva, uriWithPath,"/bar/baz", "", "/bar", "baz");
+        assertComposeEquals("Non-empty Path, null and empty inner components", kva, uriWithPath,"/foo/bar/baz", "bar", null, "", "baz");
+        assertComposeEquals("Non-empty Path, null and empty inner components", kva, uriWithPath,"/bar/baz", "/bar", null, "", "baz");
+	}
+
+	@Test
+	public void testComposeWithPathSlash() {
+
+		final URI uriWithSlashRoot = setUriPath(tempUri(), "/");
+		assertEquals("Root (/) Path", "/", uriWithSlashRoot.getPath());
+		testComposeAtLocation(uriWithSlashRoot);
+
+		final KeyValueAccess kva = newKeyValueAccess();
+		assertComposeEquals("Root (/) Path, no empty or slash in components", kva, uriWithSlashRoot, "/bar/baz", "bar", "baz");
+		assertComposeEquals("Root (/) Path, first components leading slash", kva, uriWithSlashRoot, "/bar/baz", "/bar", "baz");
+		assertComposeEquals("Root (/) Path, first components slash only", kva, uriWithSlashRoot,"/bar/baz", "/", "bar", "baz");
+		assertComposeEquals("Root (/) Path, first components slash only", kva, uriWithSlashRoot,"/bar/baz", "", "bar", "baz");
+		assertComposeEquals("Root (/) Path, first components slash only", kva, uriWithSlashRoot,"/bar/baz", "", "/bar", "baz");
+		assertComposeEquals("Root (/) Path, null and empty inner components", kva, uriWithSlashRoot,"/bar/baz", "bar", null, "", "baz");
+		assertComposeEquals("Root (/) Path, null and empty inner components", kva, uriWithSlashRoot,"/bar/baz", "/bar", null, "", "baz");
 	}
 
 	@Test
-	public void testComposeAtRoot() {
+	public void testComposeWithPathEmpty() {
+
+		final URI uriWithEmptyRoot = setUriPath(tempUri(), "");
+		assertEquals("Empty Path", "", uriWithEmptyRoot.getPath());
+		testComposeAtLocation(uriWithEmptyRoot);
+
+		final KeyValueAccess kva = newKeyValueAccess();
+		assertComposeEquals("Root (/) Path, no empty or slash in components", kva, uriWithEmptyRoot, "/bar/baz", "bar", "baz");
+		assertComposeEquals("Root (/) Path, first components leading slash", kva, uriWithEmptyRoot, "/bar/baz", "/bar", "baz");
+		assertComposeEquals("Root (/) Path, first components slash only", kva, uriWithEmptyRoot,"/bar/baz", "/", "bar", "baz");
+		assertComposeEquals("Root (/) Path, first components slash only", kva, uriWithEmptyRoot,"/bar/baz", "", "bar", "baz");
+		assertComposeEquals("Root (/) Path, first components slash only", kva, uriWithEmptyRoot,"/bar/baz", "", "/bar", "baz");
+		assertComposeEquals("Root (/) Path, null and empty inner components", kva, uriWithEmptyRoot,"/bar/baz", "bar", null, "", "baz");
+		assertComposeEquals("Root (/) Path, null and empty inner components", kva, uriWithEmptyRoot,"/bar/baz", "/bar", null, "", "baz");
+	}
+
+	public void assertComposeEquals(String reason, KeyValueAccess kva, URI uri, String absoluteExpectedPath, String... components) {
+		String actual = kva.compose(uri, components);
+		String expected = kva.compose(uri.resolve(absoluteExpectedPath));
+		assertEquals(reason, expected, actual);
+	}
+
+	public URI setUriPath(final URI uri, final String path) {
 
-		final URI root = tempUri().resolve("/");
-		testComposeAtLocation(root);
+		final URI tempUri = uri.resolve("/");
+		final String newUri = tempUri.toString().replaceAll(tempUri.getPath() + "$", path);
+		final URI uriWithNewPath = N5URI.getAsUri(newUri);
+		assertEquals("setUriPath failed", path, uriWithNewPath.getPath());
+		return uriWithNewPath;
 	}
 }

src/test/java/org/janelia/saalfeldlab/n5/kva/FileSystemKeyValueAccessTest.java

@@ -9,6 +9,7 @@
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -17,6 +18,8 @@
 import org.janelia.saalfeldlab.n5.FileSystemKeyValueAccess;
 import org.janelia.saalfeldlab.n5.KeyValueAccess;
 import org.janelia.saalfeldlab.n5.N5URI;
+import org.junit.Ignore;
+import org.junit.Test;
 
 /**
  * @author Stephan Saalfeld <[email protected]>
@@ -162,4 +165,22 @@ protected void testComposeAtLocation(URI uri) {
 			assertEquals("Failure at Index " + i , testPath, composedKey);
 		}
 	}
+
+	@Override
+	@Test
+	@Ignore("Empty path is invalid for file URIs.")
+	public void testComponentsWithPathEmpty() {
+		/* file URIs are purely paths (optional file: scheme) so empty path resolves to a relative path (not the root of the container).
+		 * Because of that, there is no valid file URI with an empty path (it's just an empty string, which is invalid, or `file://` which is invalid. */
+		super.testComponentsWithPathEmpty();
+	}
+
+	@Override
+	@Test
+	@Ignore("Empty path is invalid for file URIs.")
+	public void testComposeWithPathEmpty() {
+		/* file URIs are purely paths (optional file: scheme) so empty path resolves to a relative path (not the root of the container).
+		 * Because of that, there is no valid file URI with an empty path (it's just an empty string, which is invalid, or `file://` which is invalid. */
+		super.testComposeWithPathEmpty();
+	}
 }