HDFS-14369. RBF: Fix trailing / for webhdfs. Contributed by Akira Ajisaka.

hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MountTableResolver.java

@@ -57,6 +57,7 @@ import org.apache.hadoop.hdfs.server.federation.store.StateStoreUnavailableExcep
 import org.apache.hadoop.hdfs.server.federation.store.protocol.GetMountTableEntriesRequest;
 import org.apache.hadoop.hdfs.server.federation.store.protocol.GetMountTableEntriesResponse;
 import org.apache.hadoop.hdfs.server.federation.store.records.MountTable;
+import org.apache.hadoop.hdfs.tools.federation.RouterAdmin;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -399,12 +400,13 @@ public class MountTableResolver
   /**
    * Build the path location to insert into the cache atomically. It must hold
    * the read lock.
-   * @param path Path to check/insert.
+   * @param str Path to check/insert.
    * @return New remote location.
    * @throws IOException If it cannot find the location.
    */
-  public PathLocation lookupLocation(final String path) throws IOException {
+  public PathLocation lookupLocation(final String str) throws IOException {
     PathLocation ret = null;
+    final String path = RouterAdmin.normalizeFileSystemPath(str);
     MountTable entry = findDeepest(path);
     if (entry != null) {
       ret = buildLocation(path, entry);
@@ -432,12 +434,13 @@ public class MountTableResolver
    */
   public MountTable getMountPoint(final String path) throws IOException {
     verifyMountTable();
-    return findDeepest(path);
+    return findDeepest(RouterAdmin.normalizeFileSystemPath(path));
   }
 
   @Override
-  public List<String> getMountPoints(final String path) throws IOException {
+  public List<String> getMountPoints(final String str) throws IOException {
     verifyMountTable();
+    final String path = RouterAdmin.normalizeFileSystemPath(str);
 
     Set<String> children = new TreeSet<>();
     readLock.lock();
@@ -493,8 +496,7 @@ public class MountTableResolver
    */
   public List<MountTable> getMounts(final String path) throws IOException {
     verifyMountTable();
-
-    return getTreeValues(path, false);
+    return getTreeValues(RouterAdmin.normalizeFileSystemPath(path), false);
   }
 
   /**

hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java

@@ -26,12 +26,12 @@ import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.regex.Pattern;
 
 import org.apache.hadoop.classification.InterfaceAudience.Private;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.conf.Configured;
 import org.apache.hadoop.fs.CommonConfigurationKeys;
-import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hdfs.DFSConfigKeys;
 import org.apache.hadoop.hdfs.HdfsConfiguration;
@@ -93,6 +93,9 @@ public class RouterAdmin extends Configured implements Tool {
 
   private RouterClient client;
 
+  /** Pre-compiled regular expressions to detect duplicated slashes. */
+  private static final Pattern SLASHES = Pattern.compile("/+");
+
   public static void main(String[] argv) throws Exception {
     Configuration conf = new HdfsConfiguration();
     RouterAdmin admin = new RouterAdmin(conf);
@@ -1062,12 +1065,15 @@ public class RouterAdmin extends Configured implements Tool {
   /**
    * Normalize a path for that filesystem.
    *
-   * @param path Path to normalize.
+   * @param str Path to normalize. The path doesn't have scheme or authority.
    * @return Normalized path.
    */
-  private static String normalizeFileSystemPath(final String path) {
-    Path normalizedPath = new Path(path);
-    return normalizedPath.toString();
+  public static String normalizeFileSystemPath(final String str) {
+    String path = SLASHES.matcher(str).replaceAll("/");
+    if (path.length() > 1 && path.endsWith("/")) {
+      path = path.substring(0, path.length()-1);
+    }
+    return path;
   }
 
   /**

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/resolver/TestMountTableResolver.java

@@ -184,6 +184,23 @@ public class TestMountTableResolver {
 
   }
 
+  @Test
+  public void testDestinationOfConsecutiveSlash() throws IOException {
+    // Check files
+    assertEquals("1->/tesfile1.txt",
+        mountTable.getDestinationForPath("//tesfile1.txt///").toString());
+
+    assertEquals("3->/user/testfile2.txt",
+        mountTable.getDestinationForPath("/user///testfile2.txt").toString());
+
+    assertEquals("2->/user/test/testfile3.txt",
+        mountTable.getDestinationForPath("///user/a/testfile3.txt").toString());
+
+    assertEquals("3->/user/b/testfile4.txt",
+        mountTable.getDestinationForPath("/user/b/testfile4.txt//").toString());
+  }
+
+
   @Test
   public void testDefaultNameServiceEnable() throws IOException {
     assertTrue(mountTable.isDefaultNSEnable());
@@ -232,62 +249,120 @@ public class TestMountTableResolver {
     // Check get the mount table entry for a path
     MountTable mtEntry;
     mtEntry = mountTable.getMountPoint("/");
-    assertTrue(mtEntry.getSourcePath().equals("/"));
+    assertEquals("/", mtEntry.getSourcePath());
 
     mtEntry = mountTable.getMountPoint("/user");
-    assertTrue(mtEntry.getSourcePath().equals("/user"));
+    assertEquals("/user", mtEntry.getSourcePath());
 
     mtEntry = mountTable.getMountPoint("/user/a");
-    assertTrue(mtEntry.getSourcePath().equals("/user/a"));
+    assertEquals("/user/a", mtEntry.getSourcePath());
 
     mtEntry = mountTable.getMountPoint("/user/a/");
-    assertTrue(mtEntry.getSourcePath().equals("/user/a"));
+    assertEquals("/user/a", mtEntry.getSourcePath());
 
     mtEntry = mountTable.getMountPoint("/user/a/11");
-    assertTrue(mtEntry.getSourcePath().equals("/user/a"));
+    assertEquals("/user/a", mtEntry.getSourcePath());
 
     mtEntry = mountTable.getMountPoint("/user/a1");
-    assertTrue(mtEntry.getSourcePath().equals("/user"));
+    assertEquals("/user", mtEntry.getSourcePath());
+  }
+
+  @Test
+  public void testGetMountPointOfConsecutiveSlashes() throws IOException {
+    // Check get the mount table entry for a path
+    MountTable mtEntry;
+    mtEntry = mountTable.getMountPoint("///");
+    assertEquals("/", mtEntry.getSourcePath());
+
+    mtEntry = mountTable.getMountPoint("///user//");
+    assertEquals("/user", mtEntry.getSourcePath());
+
+    mtEntry = mountTable.getMountPoint("/user///a");
+    assertEquals("/user/a", mtEntry.getSourcePath());
+
+    mtEntry = mountTable.getMountPoint("/user/a////");
+    assertEquals("/user/a", mtEntry.getSourcePath());
+
+    mtEntry = mountTable.getMountPoint("///user/a/11//");
+    assertEquals("/user/a", mtEntry.getSourcePath());
+
+    mtEntry = mountTable.getMountPoint("/user///a1///");
+    assertEquals("/user", mtEntry.getSourcePath());
+  }
+
+  @Test
+  public void testTrailingSlashInInputPath() throws IOException {
+    // Check mount points beneath the path with trailing slash.
+    getMountPoints(true);
   }
 
   @Test
   public void testGetMountPoints() throws IOException {
+    // Check mount points beneath the path without trailing slash.
+    getMountPoints(false);
+  }
 
+  private void getMountPoints(boolean trailingSlash) throws IOException {
     // Check getting all mount points (virtual and real) beneath a path
     List<String> mounts = mountTable.getMountPoints("/");
     assertEquals(5, mounts.size());
     compareLists(mounts, new String[] {"tmp", "user", "usr",
         "readonly", "multi"});
 
-    mounts = mountTable.getMountPoints("/user");
+    String path = trailingSlash ? "/user/" : "/user";
+    mounts = mountTable.getMountPoints(path);
     assertEquals(2, mounts.size());
     compareLists(mounts, new String[] {"a", "b"});
 
-    mounts = mountTable.getMountPoints("/user/a");
+    path = trailingSlash ? "/user/a/" : "/user/a";
+    mounts = mountTable.getMountPoints(path);
     assertEquals(1, mounts.size());
     compareLists(mounts, new String[] {"demo"});
 
-    mounts = mountTable.getMountPoints("/user/a/demo");
+    path = trailingSlash ? "/user/a/demo/" : "/user/a/demo";
+    mounts = mountTable.getMountPoints(path);
     assertEquals(1, mounts.size());
     compareLists(mounts, new String[] {"test"});
 
-    mounts = mountTable.getMountPoints("/user/a/demo/test");
+    path = trailingSlash ? "/user/a/demo/test/" : "/user/a/demo/test";
+    mounts = mountTable.getMountPoints(path);
     assertEquals(2, mounts.size());
     compareLists(mounts, new String[] {"a", "b"});
 
-    mounts = mountTable.getMountPoints("/tmp");
+    path = trailingSlash ? "/tmp/" : "/tmp";
+    mounts = mountTable.getMountPoints(path);
     assertEquals(0, mounts.size());
 
-    mounts = mountTable.getMountPoints("/t");
+    path = trailingSlash ? "/t/" : "/t";
+    mounts = mountTable.getMountPoints(path);
     assertNull(mounts);
 
-    mounts = mountTable.getMountPoints("/unknownpath");
+    path = trailingSlash ? "/unknownpath/" : "/unknownpath";
+    mounts = mountTable.getMountPoints(path);
     assertNull(mounts);
 
-    mounts = mountTable.getMountPoints("/multi");
+    path = trailingSlash ? "/multi/" : "/multi";
+    mounts = mountTable.getMountPoints(path);
     assertEquals(0, mounts.size());
   }
 
+  @Test
+  public void testSuccessiveSlashesInInputPath() throws IOException {
+    // Check getting all mount points (virtual and real) beneath a path
+    List<String> mounts = mountTable.getMountPoints("///");
+    assertEquals(5, mounts.size());
+    compareLists(mounts, new String[] {"tmp", "user", "usr",
+        "readonly", "multi"});
+    String path = "///user//";
+    mounts = mountTable.getMountPoints(path);
+    assertEquals(2, mounts.size());
+    compareLists(mounts, new String[] {"a", "b"});
+    path = "/user///a";
+    mounts = mountTable.getMountPoints(path);
+    assertEquals(1, mounts.size());
+    compareLists(mounts, new String[] {"demo"});
+  }
+
   private void compareRecords(List<MountTable> list1, String[] list2) {
     assertEquals(list1.size(), list2.length);
     for (String item : list2) {
@@ -334,6 +409,26 @@ public class TestMountTableResolver {
     compareRecords(records, new String[] {"/multi"});
   }
 
+  @Test
+  public void testGetMountsOfConsecutiveSlashes() throws IOException {
+    // Check listing the mount table records at or beneath a path
+    List<MountTable> records = mountTable.getMounts("///");
+    assertEquals(10, records.size());
+    compareRecords(records, new String[] {"/", "/tmp", "/user", "/usr/bin",
+        "user/a", "/user/a/demo/a", "/user/a/demo/b", "/user/b/file1.txt",
+        "readonly", "multi"});
+
+    records = mountTable.getMounts("/user///");
+    assertEquals(5, records.size());
+    compareRecords(records, new String[] {"/user", "/user/a/demo/a",
+        "/user/a/demo/b", "user/a", "/user/b/file1.txt"});
+
+    records = mountTable.getMounts("///user///a");
+    assertEquals(3, records.size());
+    compareRecords(records,
+        new String[] {"/user/a/demo/a", "/user/a/demo/b", "/user/a"});
+  }
+
   @Test
   public void testRemoveSubTree()
       throws UnsupportedOperationException, IOException {

hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterMountTable.java

@@ -442,4 +442,36 @@ public class TestRouterMountTable {
         "Directory/File does not exist /mount/file",
         () -> routerFs.setOwner(new Path("/mount/file"), "user", "group"));
   }
+
+  /**
+   * Regression test for HDFS-14369.
+   * Verify that getListing works with the path with trailing slash.
+   */
+  @Test
+  public void testGetListingWithTrailingSlash() throws IOException {
+    try {
+      // Add mount table entry
+      MountTable addEntry = MountTable.newInstance("/testlist",
+          Collections.singletonMap("ns0", "/testlist"));
+      assertTrue(addMountTable(addEntry));
+      addEntry = MountTable.newInstance("/testlist/tmp0",
+          Collections.singletonMap("ns0", "/testlist/tmp0"));
+      assertTrue(addMountTable(addEntry));
+      addEntry = MountTable.newInstance("/testlist/tmp1",
+          Collections.singletonMap("ns1", "/testlist/tmp1"));
+      assertTrue(addMountTable(addEntry));
+
+      nnFs0.mkdirs(new Path("/testlist/tmp0"));
+      nnFs1.mkdirs(new Path("/testlist/tmp1"));
+      // Fetch listing
+      DirectoryListing list = routerProtocol.getListing(
+          "/testlist/", HdfsFileStatus.EMPTY_NAME, false);
+      HdfsFileStatus[] statuses = list.getPartialListing();
+      // should return tmp0 and tmp1
+      assertEquals(2, statuses.length);
+    } finally {
+      nnFs0.delete(new Path("/testlist/tmp0"), true);
+      nnFs1.delete(new Path("/testlist/tmp1"), true);
+    }
+  }
 }