HADOOP-5901. FileSystem.fixName() has unexpected behaviour. Contributed by Aaron Kimball.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@890651 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -94,6 +94,9 @@ Trunk (unreleased changes)
     HADOOP-6391. Classpath should not be part of command line arguments.
     (Cristian Ivascu via tomwhite)
 
+    HADOOP-5901. FileSystem.fixName() has unexpected behaviour.
+    (Aaron Kimball via tomwhite)
+
 Release 0.21.0 - Unreleased
 
   INCOMPATIBLE CHANGES

src/java/org/apache/hadoop/fs/FileSystem.java

@@ -21,6 +21,7 @@ import java.io.Closeable;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.EnumSet;
@@ -108,7 +109,15 @@ public abstract class FileSystem extends Configured implements Closeable {
    * @return the uri of the default filesystem
    */
   public static URI getDefaultUri(Configuration conf) {
-    return URI.create(fixName(conf.get(FS_DEFAULT_NAME_KEY, DEFAULT_FS)));
+    try {
+      String uri = conf.get(FS_DEFAULT_NAME_KEY, null);
+      checkName(uri);
+      return new URI(uri);
+    } catch (Exception e) {
+      // fs.default.name not set, or set to an invalid value. Create
+      // one based on a known-good URI
+      return URI.create(DEFAULT_FS);
+    }
   }
 
   /** Set the default filesystem URI in a configuration.
@@ -122,9 +131,12 @@ public abstract class FileSystem extends Configured implements Closeable {
   /** Set the default filesystem URI in a configuration.
    * @param conf the configuration to alter
    * @param uri the new default filesystem uri
+   * @throws IOException if the URI is invalid.
    */
-  public static void setDefaultUri(Configuration conf, String uri) {
-    setDefaultUri(conf, URI.create(fixName(uri)));
+  public static void setDefaultUri(Configuration conf, String uri)
+      throws IOException {
+    checkName(uri);
+    setDefaultUri(conf, URI.create(uri));
   }
 
   /** Called after a new FileSystem instance is constructed.
@@ -138,22 +150,52 @@ public abstract class FileSystem extends Configured implements Closeable {
 
   /** Returns a URI whose scheme and authority identify this FileSystem.*/
   public abstract URI getUri();
-  
-  /** Update old-format filesystem names, for back-compatibility.  This should
-   * eventually be replaced with a checkName() method that throws an exception
-   * for old-format names. */ 
-  private static String fixName(String name) {
-    // convert old-format name to new-format name
-    if (name.equals("local")) {         // "local" is now "file:///".
-      LOG.warn("\"local\" is a deprecated filesystem name."
-               +" Use \"file:///\" instead.");
-      name = "file:///";
-    } else if (name.indexOf('/')==-1) {   // unqualified is "hdfs://"
-      LOG.warn("\""+name+"\" is a deprecated filesystem name."
-               +" Use \"hdfs://"+name+"/\" instead.");
-      name = "hdfs://"+name;
-    }
-    return name;
+
+  /** Checks that a FileSystem name is given in an understandable format.
+   * The old "local" alias for "file:///" is unsupported, as are any
+   * URIs without a scheme component.
+   * @throws IOException if a name is in an unsupported format
+   */
+  private static void checkName(String name) throws IOException {
+    if (null == name) {
+      throw new IOException("Null FS name provided to checkName()");
+    } else if ("local".equals(name)) {
+      throw new IOException ("FileSystem 'local' is not supported; use 'file:///'");
+    } else {
+      // Try parsing this into a URI
+      try {
+        URI uri = new URI(name);
+
+        // No scheme; don't know how to parse this.
+        if (null == uri.getScheme()) {
+          throw new IOException("FileSystem name '" + name
+              + "' is provided in an unsupported format. (Try 'hdfs://"
+              + name + "' instead?)");
+        }
+
+        // This may have been a misparse. java.net.URI specifies that
+        // a URI is of the form:
+        // URI ::= [SCHEME-PART:]SCHEME-SPECIFIC-PART
+        //
+        // The scheme-specific-part may be parsed in numerous ways, but if
+        // it starts with a '/' character, that makes it a "hierarchical URI",
+        // subject to the following parsing:
+        // SCHEME-SPECIFIC-PART ::= "//" AUTHORITY-PART
+        // AUTHORITY-PART ::= [USER-INFO-PART] HOSTNAME [ ":" PORT ]
+        //
+        // In Hadoop, we require a host-based authority as well.
+        // java.net.URI parses left-to-right, so deprecated hostnames of the
+        // form 'foo:8020' will have 'foo' as their scheme and '8020' as their
+        // scheme-specific-part. We don't want this behavior.
+        if (null == uri.getAuthority()) {
+          throw new IOException("FileSystem name '" + name
+              + "' is provided in an unsupported format. (Try 'hdfs://"
+              + name + "' instead?)");
+        }
+      } catch (URISyntaxException use) {
+        throw new IOException("FileSystem name cannot be understood as a URI", use);
+      }
+    }
   }
 
   /**

src/test/core/org/apache/hadoop/fs/TestFileSystemCaching.java

@@ -18,8 +18,12 @@
 
 package org.apache.hadoop.fs;
 
+import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertSame;
+import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.assertNotSame;
+import static junit.framework.Assert.assertTrue;
+import static junit.framework.Assert.fail;
 
 import java.net.URI;
 
@@ -47,4 +51,94 @@ public class TestFileSystemCaching {
     assertNotSame(fs1, fs2);
   }
 
+  @Test
+  public void testGetLocal() throws Exception {
+    Configuration conf = new Configuration();
+    conf.set(CommonConfigurationKeys.FS_DEFAULT_NAME_KEY,
+        CommonConfigurationKeys.FS_DEFAULT_NAME_DEFAULT);
+    FileSystem fs1 = FileSystem.get(conf);
+    assertTrue(fs1 instanceof LocalFileSystem);
+
+    FileSystem fs2 = FileSystem.get(LocalFileSystem.NAME, conf);
+    assertTrue(fs2 instanceof LocalFileSystem);
+  }
+
+  @Test
+  public void testGetDefaultFs() throws Exception {
+    Configuration conf = new Configuration();
+    FileSystem fs = FileSystem.get(conf);
+    assertNotNull(fs);
+  }
+
+  private void checkDefaultFsParam(Configuration conf, String uri)
+      throws Exception {
+    conf.set(CommonConfigurationKeys.FS_DEFAULT_NAME_KEY, uri);
+    assertNotNull(FileSystem.get(conf));
+  }
+
+  @Test
+  public void testInvalidDefaultFsParam() throws Exception {
+    Configuration conf = new Configuration();
+
+    // All of the following set the default filesystem config
+    // to an invalid URI. Subsequent requests for a FileSystem
+    // should return the internal default fs.
+
+    checkDefaultFsParam(conf, "this-is-an-/invalid_uri");
+    checkDefaultFsParam(conf, "");
+    checkDefaultFsParam(conf, "/foo");
+    checkDefaultFsParam(conf, "/foo/bar");
+    checkDefaultFsParam(conf, "foo");
+    checkDefaultFsParam(conf, "foo:8020");
+    checkDefaultFsParam(conf, "foo/bar");
+    checkDefaultFsParam(conf, "foo:8020/bar");
+    checkDefaultFsParam(conf, "hdfs://");
+    checkDefaultFsParam(conf, "local");
+  }
+
+  @Test
+  public void testGetInvalidFs() throws Exception {
+    Configuration conf = new Configuration();
+
+    // None of these are valid FileSystem URIs. The default FS
+    // should be returned in all cases.
+
+    assertNotNull(FileSystem.get(URI.create("/foo"), conf));
+    assertNotNull(FileSystem.get(URI.create("/foo/bar"), conf));
+    assertNotNull(FileSystem.get(URI.create("foo"), conf));
+    assertNotNull(FileSystem.get(URI.create("foo/bar"), conf));
+    assertNotNull(FileSystem.get(URI.create("local"), conf));
+  }
+
+  private void checkBadUri(Configuration conf, String uri) {
+    try {
+      FileSystem.setDefaultUri(conf, uri);
+      fail("Expected invalid URI: " + uri);
+    } catch (Exception e) {
+      // Got expected exception; ok.
+    }
+  }
+
+  @Test
+  public void testDefaultUri() throws Exception {
+    Configuration conf = new Configuration();
+    URI uri = FileSystem.getDefaultUri(conf);
+    assertNotNull(uri.getScheme());
+
+    final URI exampleGoodUri = new URI("hdfs://foo.example.com:999");
+
+    FileSystem.setDefaultUri(conf, exampleGoodUri);
+    URI out = FileSystem.getDefaultUri(conf);
+    assertEquals(exampleGoodUri, out);
+
+    checkBadUri(conf, "bla"); // no scheme
+    checkBadUri(conf, "local"); // deprecated syntax
+    checkBadUri(conf, "foo:8020"); // no scheme, deprecated syntax.
+    checkBadUri(conf, "hdfs://"); // not a valid uri; requires authority-part
+    checkBadUri(conf, ""); // not a uri.
+
+    // Check that none of these actually changed the conf.
+    out = FileSystem.getDefaultUri(conf);
+    assertEquals(exampleGoodUri, out);
+  }
 }