Merge pull request #2 from TimWolters/3188-make-resolve-more-robust

make relative S3Path.resolve correct

Author: sbeimin

src/main/java/com/upplication/s3fs/S3Path.java

@@ -1,22 +1,27 @@
 package com.upplication.s3fs;
 
-import com.google.common.base.*;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Lists;
-import com.upplication.s3fs.attribute.S3BasicFileAttributes;
+import static com.google.common.collect.Iterables.concat;
+import static java.lang.String.format;
 
 import java.io.File;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URL;
 import java.net.URLDecoder;
-import java.nio.file.*;
+import java.nio.file.LinkOption;
+import java.nio.file.Path;
+import java.nio.file.WatchEvent;
+import java.nio.file.WatchKey;
+import java.nio.file.WatchService;
 import java.util.Iterator;
 import java.util.List;
 
-import static com.google.common.collect.Iterables.*;
-import static java.lang.String.format;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import com.upplication.s3fs.attribute.S3BasicFileAttributes;
 
 public class S3Path implements Path {
 
@@ -353,27 +358,37 @@ public Path normalize() {
 
     @Override
     public Path resolve(Path other) {
+		String otherUri = "";
         if (other.isAbsolute()) {
-            Preconditions.checkArgument(other instanceof S3Path, "other must be an instance of %s", S3Path.class.getName());
+            Preconditions.checkArgument(other instanceof S3Path, "other must be an instance of %s or be relative", S3Path.class.getName());
             return other;
+		} else if (!(other instanceof S3Path)) {
+			int nameCount = other.getNameCount();
+			for (int i = 0; i < nameCount; i++) {
+				if (i > 0)
+					otherUri += PATH_SEPARATOR;
+				otherUri += other.getName(i);
+			}
+		} else {
+			S3Path otherS3Path = (S3Path) other;
+			otherUri = otherS3Path.uri;
         }
 
-        S3Path otherS3Path = (S3Path) other;
         StringBuilder pathBuilder = new StringBuilder();
 
         if (this.isAbsolute()) {
             pathBuilder.append(PATH_SEPARATOR + this.fileStore.name() + PATH_SEPARATOR);
         }
         pathBuilder.append(this.uri);
-        if (!otherS3Path.uri.isEmpty())
-            pathBuilder.append(PATH_SEPARATOR + otherS3Path.uri);
+		if (!otherUri.isEmpty())
+			pathBuilder.append(PATH_SEPARATOR + otherUri);
 
         return new S3Path(this.fileSystem, pathBuilder.toString());
     }
 
     @Override
     public Path resolve(String other) {
-        return resolve(new S3Path(this.getFileSystem(), other));
+		return resolve(new S3Path(this.getFileSystem(), URI.create(other).getPath()));
     }
 
     @Override

src/test/java/com/upplication/s3fs/Path/S3PathTest.java

@@ -1,5 +1,6 @@
 package com.upplication.s3fs.Path;
 
+import com.upplication.s3fs.AmazonS3Factory;
 import com.upplication.s3fs.S3FileSystem;
 import com.upplication.s3fs.S3FileSystemProvider;
 import com.upplication.s3fs.S3Path;
@@ -9,8 +10,10 @@
 import org.junit.Test;
 
 import java.io.IOException;
+import java.net.URI;
 import java.nio.file.FileSystems;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.nio.file.WatchEvent;
 import java.util.HashMap;
 
@@ -232,4 +235,55 @@ public void registerWatchService() throws IOException {
         S3Path path = forPath("/buck/file");
         path.register(null, new WatchEvent.Kind<?>[0], new WatchEvent.Modifier[0]);
     }
+	@Test
+	public void testResolve() throws IOException {
+		S3FileSystem fileSystem2 = null;
+		try {
+			HashMap<String, String> environments = new HashMap<>();
+			environments.put(AmazonS3Factory.ACCESS_KEY, "accesskey");
+			environments.put(AmazonS3Factory.SECRET_KEY, "secretaccesskey");
+
+			fileSystem2 = (S3FileSystem) FileSystems.newFileSystem(URI.create("s3://accesskey:[email protected]/bucket"), environments);
+
+			//good
+			S3Path parent = (S3Path) fileSystem2.provider().getPath(URI.create("s3://accesskey:[email protected]/bucket"));
+			S3Path child = (S3Path) fileSystem2.provider().getPath(URI.create("s3://accesskey:[email protected]/bucket/rabbit"));
+			S3Path resolved = (S3Path) parent.resolve(child);
+
+			assertEquals(child, resolved);
+
+			resolved = (S3Path) parent.resolve("s3://accesskey:[email protected]/bucket/rabbit");
+			assertEquals(child, resolved);
+
+			resolved = (S3Path) parent.resolve("rabbit");
+			assertEquals(child, resolved);
+
+			resolved = (S3Path) parent.resolve(Paths.get("rabbit")); //unixPath
+			assertEquals(child, resolved);
+
+			resolved = (S3Path) parent.resolve(Paths.get("./rabbit")); //unixPath
+			assertEquals("s3://accesskey:[email protected]/bucket/./rabbit", resolved.toString());
+
+			resolved = (S3Path) parent.resolve(Paths.get("./rabbit in space")); //unixPath
+			assertEquals("s3://accesskey:[email protected]/bucket/./rabbit%20in%20space", resolved.toString());
+
+			try {
+				parent.resolve(Paths.get("/tmp"));
+				fail("expect IllegalArgumentException");
+			} catch (IllegalArgumentException e) {
+				//ignore
+				assertEquals("other must be an instance of com.upplication.s3fs.S3Path or be relative", e.getMessage());
+			}
+
+			//bad
+			S3Path parent2 = fileSystem2.getPath("s3://accesskey:[email protected]/bucket");
+			S3Path child2 = fileSystem2.getPath("s3://accesskey:[email protected]/bucket/rabbit");
+			S3Path resolved2 = (S3Path) parent2.resolve(child2);
+			assertEquals("s3:/accesskey:[email protected]/bucket/s3:/accesskey:[email protected]/bucket/rabbit", resolved2.toString());
+
+		} finally {
+			if (fileSystem2 != null)
+				fileSystem2.close();
+		}
+	}
 }

src/test/java/com/upplication/s3fs/Path/ToUriTest.java

@@ -77,6 +77,9 @@ public void toUriRelative() {
 
         S3Path path = new S3Path(fileSystem, "bla");
         assertEquals(URI.create("bla"), path.toUri());
+        
+        S3Path withSpaces = new S3Path(fileSystem, "with space");
+        assertEquals(URI.create("with%20space"), withSpaces.toUri());
     }
 
     @Test