Do "path normalization" when creating a URI.

sdk/lib/core/uri.dart

Index: sdk/lib/core/uri.dart
diff --git a/sdk/lib/core/uri.dart b/sdk/lib/core/uri.dart
index 5cf49b07ff44958a6877187ffa03a5ede3e163d3..4e15238f0b93321b3f9e3510e2ae0cea0d5225ea 100644
--- a/sdk/lib/core/uri.dart
+++ b/sdk/lib/core/uri.dart
@@ -381,9 +381,8 @@ class Uri {
     }
 
     assert(state == NOT_IN_PATH);
-    bool isFile = (scheme == "file");
-    bool ensureLeadingSlash = host != null;
-    path = _makePath(uri, pathStart, index, null, ensureLeadingSlash, isFile);
+    bool hasAuthority = host != null;

[floitsch, 2015/07/17 13:03:20]

nit: (host != null)

[Lasse Reichstein Nielsen, 2015/07/17 18:40:22]

Done.

+    path = _makePath(uri, pathStart, index, null, scheme, hasAuthority);
 
     if (char == _QUESTION) {
       int numberSignIndex = -1;
@@ -511,9 +510,14 @@ class Uri {
         (userInfo.isNotEmpty || port != null || isFile)) {
       host = "";
     }
-    bool ensureLeadingSlash = host != null;
+    bool hasAuthority = host != null;

[floitsch, 2015/07/17 13:03:20]

ditto.

     path = _makePath(path, 0, _stringOrNullLength(path), pathSegments,
-                     ensureLeadingSlash, isFile);
+                     scheme, hasAuthority);
+    if (scheme.isEmpty && host == null && !path.startsWith('/')) {

[floitsch, 2015/07/17 13:03:20]

ditto.

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

Not here, no.
Reading expressions between && is not improved by extra wrapping. The && is a
very visible separator already, and adding parentheses only makes it harder to
read because then you have to find the matching one before you can separate the
expression into parts.

+      path = _normalizeRelativePath(path);
+    } else {
+      path = _removeDotSegments(path);
+    }
     return new Uri._internal(scheme, userInfo, host, port,
                              path, query, fragment);
   }
@@ -953,15 +957,15 @@ class Uri {
       host = "";
     }
 
-    bool ensureLeadingSlash = (host != null);
+    bool hasAuthority = host != null;

[floitsch, 2015/07/17 13:03:20]

we seem to disagree :)
I like the parenthesis.

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

We do disagree. 
Parentheses are added to change or highlight the associativity of operators.
If you add them when they are not necessary, you require the reader to match up
the parentheses before parsing their content. 
In some settings, that's ok because the associativity isn't very clear (nobody
remembers '|' vs '<') so you add them as emphasis and explanation.
In other cases, there should be no doubt about the meaning, and adding
parenthses then only encumer the reader who might think they are added for a
reason that he can't see.

I wouldn't add parentheses around "var sum = something + other;", so I didn't
add parentheses around "host != null". The one reason for doing it is that the
operator contains a '=' sign which may confuse reading slighly.

I'll add it here, and in similar cases, because it's simple enough that I don't
think it matters either way, but in general I don't think adding parntheses is
without cost, and the cost need to be outweighed by benefits.

     if (path != null || pathSegments != null) {
       path = _makePath(path, 0, _stringOrNullLength(path), pathSegments,
-                       ensureLeadingSlash, isFile);
+                       scheme, hasAuthority);
     } else {
       path = this.path;
-      if ((isFile || (ensureLeadingSlash && !path.isEmpty)) &&
+      if ((isFile || (hasAuthority && !path.isEmpty)) &&
           !path.startsWith('/')) {
-        path = "/$path";
+        path = "/" + path;
       }
     }
 
@@ -1029,16 +1033,16 @@ class Uri {
    *
    * A normalized path does not contain `.` segments or non-leading `..`
    * segments.
-   * Only a relative path may contain leading `..` segments,
+   * Only a relative path with no scheme or authority may contain
+   * leading `..` segments,
    * a path that starts with `/` will also drop any leading `..` segments.
    *
-   * This uses the same normalization strategy as [resolveUri], as specified by
-   * RFC 3986.
+   * This uses the same normalization strategy as `new Uri().resolve(this)`.
    *
    * Does not change any part of the URI except the path.
    */
   Uri normalizePath() {
-    String path = _removeDotSegments(_path);
+    String path = _normalizePath(_path, scheme, hasAuthority);
     if (identical(path, _path)) return this;
     return this.replace(path: path);
   }
@@ -1178,17 +1182,18 @@ class Uri {
     if (!_isAlphabeticCharacter(firstCodeUnit)) {
       _fail(scheme, start, "Scheme not starting with alphabetic character");
     }
-    bool allLowercase = firstCodeUnit >= _LOWER_CASE_A;
+    int minChar = firstCodeUnit;
     for (int i = start; i < end; i++) {
       final int codeUnit = scheme.codeUnitAt(i);
       if (!_isSchemeCharacter(codeUnit)) {
         _fail(scheme, i, "Illegal scheme character");
       }
-      if (codeUnit < _LOWER_CASE_A || codeUnit > _LOWER_CASE_Z) {
-        allLowercase = false;
-      }
+      if (codeUnit < minChar) minChar = codeUnit;
     }
+    // All valid scheme characters are greater than or equal to _UPPER_CASE_A.
+    // If they are above _UPPER_CASE_Z, the scheme is already lower-case.

[floitsch, 2015/07/17 13:03:20]

maybe add assert(_UPPER_CASE_Z < _LOWER_CASE_A);

(for documentation purposes)

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

I'll revert this, I had missed that there are valid non-letter characters before
'A' as well.

     scheme = scheme.substring(start, end);
+    bool allLowercase = minChar > _UPPER_CASE_Z;

[floitsch, 2015/07/17 13:03:20]

why not minChar >= _LOWER_CASE_A ?

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

Because we allow two non-letter characters, one of which is between 'Z' and 'a'.
Sadly, some are also below 'A', so this doesn't work.

     if (!allLowercase) scheme = scheme.toLowerCase();
     return scheme;
   }
@@ -1200,8 +1205,10 @@ class Uri {
 
   static String _makePath(String path, int start, int end,
                           Iterable<String> pathSegments,
-                          bool ensureLeadingSlash,
-                          bool isFile) {
+                          String scheme,
+                          bool hasAuthority) {
+    bool isFile = (scheme == "file");
+    bool ensureLeadingSlash = isFile || hasAuthority;
     if (path == null && pathSegments == null) return isFile ? "/" : "";
     if (path != null && pathSegments != null) {
       throw new ArgumentError('Both path and pathSegments specified');
@@ -1214,13 +1221,25 @@ class Uri {
     }
     if (result.isEmpty) {
       if (isFile) return "/";
-    } else if ((isFile || ensureLeadingSlash) &&
-               result.codeUnitAt(0) != _SLASH) {
-      return "/$result";
+    } else if (ensureLeadingSlash && !result.startsWith('/')) {
+      result = "/" + result;
     }
+    result = _normalizePath(result, scheme, hasAuthority);
     return result;
   }
 
+  /// Performs path normalization (remove dot segments) on a path.
+  ///
+  /// If the URI has neither scheme nor authority, it's considered a
+  /// "pure path" and normalization won't remove leading ".." segments.
+  /// Otherwise it follows the RFC 3986 "remove dot segments" algorithm.
+  static String _normalizePath(String path, String scheme, bool hasAuthority) {
+    if (scheme.isEmpty && !hasAuthority && !path.startsWith('/')) {
+      return _normalizeRelativePath(path);
+    }
+    return _removeDotSegments(path);
+  }
+
   static String _makeQuery(String query, int start, int end,
                            Map<String, String> queryParameters) {
     if (query == null && queryParameters == null) return null;
@@ -1429,8 +1448,7 @@ class Uri {
    */
   bool get isAbsolute => scheme != "" && fragment == "";
 
-  String _merge(String base, String reference) {
-    if (base.isEmpty) return "/$reference";
+  String _mergePaths(String base, String reference) {
     // Optimize for the case: absolute base, reference beginning with "../".
     int backCount = 0;
     int refStart = 0;
@@ -1463,21 +1481,35 @@ class Uri {
                              reference.substring(refStart - 3 * backCount));
   }
 
-  bool _hasDotSegments(String path) {
+  /// Make a guess at whether a path contains a `..` or `.` segments.

[floitsch, 2015/07/17 13:03:20]

segment.

[Lasse Reichstein Nielsen, 2015/07/17 18:40:22]

Done.

+  ///
+  /// This is a primitive test that can cause false positives.
+  /// It's only used to avoid a more expensive operation in the case where
+  /// it's not necessary.
+  static bool _hasDotSegments(String path) {

[floitsch, 2015/07/17 13:03:20]

_maybeHasDotSegments ?

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

_mayContainDotSegments

     if (path.length > 0 && path.codeUnitAt(0) == _DOT) return true;
     int index = path.indexOf("/.");
     return index != -1;
   }
 
-  String _removeDotSegments(String path) {
+  /// Removes '.' and '..' segments from a path.
+  ///
+  /// Follows the RFC 2986 "remove dot segments" algorithm.
+  /// This algorithm is only used on paths of URIs with a scheme,
+  /// and it treats the path as if it is absolute (leading '..' are removed).
+  static String _removeDotSegments(String path) {
     if (!_hasDotSegments(path)) return path;
     List<String> output = [];
     bool appendSlash = false;
     for (String segment in path.split("/")) {
       appendSlash = false;
       if (segment == "..") {
-        if (!output.isEmpty &&
-            ((output.length != 1) || (output[0] != ""))) output.removeLast();
+        if (output.isNotEmpty) {
+          output.removeLast();
+          if (output.isEmpty) {
+            output.add("");
+          }
+        }
         appendSlash = true;
       } else if ("." == segment) {
         appendSlash = true;
@@ -1489,6 +1521,39 @@ class Uri {
     return output.join("/");
   }
 
+  /// Removes all `.` segments and any non-leading `..` segments.
+  ///
+  /// Removing the ".." from a "bar/foo/.." sequence results in "bar/"
+  /// (trailing "/"). If the entire path is removed (because it contains as
+  /// many ".." segments as real segments), the result is "./".
+  /// This is different from an empty string, which represents "no path",
+  /// when you resolve it against a base URI with a path with a non-empty
+  /// final segment.
+  static String _normalizeRelativePath(String path) {
+    assert(!path.startsWith('/'));  // Only get called for relative paths.
+    if (!_hasDotSegments(path)) return path;  // Ensures path non-empty below.

[floitsch, 2015/07/17 13:03:20]

If _hasDotSegments return false for the empty string it must say so in the
documentation.

?:
// Also weeds out empty paths.

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

Well, it's pretty certain that an empty path won't contain any dot segments, so
not something I would document on that function. That's why I document it here.
I'll just add an assert.

+    List<String> output = [];
+    bool appendSlash = false;
+    for (String segment in path.split("/")) {
+      appendSlash = false;
+      if (".." == segment) {
+        if (!output.isEmpty && output.last != "..") {
+          output.removeLast();
+          appendSlash = true;
+        } else {
+          output.add("..");
+        }
+      } else if ("." == segment) {
+        appendSlash = true;
+      } else {
+        output.add(segment);
+      }
+    }
+    if (output.isEmpty || output.length == 1 && output[0].isEmpty) return "./";

[floitsch, 2015/07/17 13:03:20]

parenthesis

[Lasse Reichstein Nielsen, 2015/07/17 18:40:22]

Ack, with mixed || and &&, some parentheses can improve reading.

+    if (appendSlash || output.last == '..') output.add("");
+    return output.join("/");
+  }
+
   /**
    * Resolve [reference] as an URI relative to `this`.
    *
@@ -1508,9 +1573,15 @@ class Uri {
    *
    * Returns the resolved URI.
    *
-   * The algorithm for resolving a reference is described in
-   * [RFC-3986 Section 5]
+   * The algorithm "Transform Reference" for resolving a reference is
+   * described in [RFC-3986 Section 5]
    * (http://tools.ietf.org/html/rfc3986#section-5 "RFC-1123").
+   *
+   * Updated to handle the case where the base URI is just a relative path -
+   * that is: when it has no scheme or authority and the path does not start
+   * with a slash.
+   * In that case, the paths are combined without removing leading "..", and
+   * an empty path is not converted to "/".
    */
   Uri resolveUri(Uri reference) {
     // From RFC 3986.
@@ -1541,6 +1612,9 @@ class Uri {
         targetPath = _removeDotSegments(reference.path);
         if (reference.hasQuery) targetQuery = reference.query;
       } else {
+        targetUserInfo = this._userInfo;
+        targetHost = this._host;
+        targetPort = this._port;
         if (reference.path == "") {
           targetPath = this._path;
           if (reference.hasQuery) {
@@ -1549,16 +1623,31 @@ class Uri {
             targetQuery = this._query;
           }
         } else {
-          if (reference.path.startsWith("/")) {
+          if (reference.hasAbsolutePath) {
             targetPath = _removeDotSegments(reference.path);
           } else {
-            targetPath = _removeDotSegments(_merge(this._path, reference.path));
+            // This is the RFC 3986 behavior for merging.
+            if (this.hasEmptyPath) {
+              if (!this.hasScheme && !this.hasAuthority) {
+                // Keep the path relative if no scheme or authority.
+                targetPath = reference.path;
+              } else {
+                // Add path normalization on top of RFC algorithm.
+                targetPath = _removeDotSegments("/" + reference.path);
+              }
+            } else {
+              var mergedPath = _mergePaths(this._path, reference.path);
+              if (this.hasScheme || this.hasAuthority || this.hasAbsolutePath) {
+                targetPath = _removeDotSegments(mergedPath);
+              } else {
+                // Non-RFC 3986 beavior. If both base and reference are relative

[floitsch, 2015/07/17 13:03:20]

behavior.

Also state if this is violating the RFC, or if it is additional behavior that
isn't covered by the RFC.

[Lasse Reichstein Nielsen, 2015/07/17 18:40:23]

Not covered, will document.

+                // path, allow the merged path to start with "..".
+                targetPath = _normalizeRelativePath(mergedPath);
+              }
+            }
           }
           if (reference.hasQuery) targetQuery = reference.query;
         }
-        targetUserInfo = this._userInfo;
-        targetHost = this._host;
-        targetPort = this._port;
       }
     }
     String fragment = reference.hasFragment ? reference.fragment : null;
@@ -1572,6 +1661,11 @@ class Uri {
   }
 
   /**
+   * Returns whether the URI has a [scheme] component.
+   */
+  bool get hasScheme => scheme.isNotEmpty;
+
+  /**
    * Returns whether the URI has an [authority] component.
    */
   bool get hasAuthority => _host != null;
@@ -1597,6 +1691,16 @@ class Uri {
   bool get hasFragment => _fragment != null;
 
   /**
+   * Returns whether the URI has an empty path.
+   */
+  bool get hasEmptyPath => _path.isEmpty;
+
+  /**
+   * Returns whether the URI has an absolute path (starting with '/').
+   */
+  bool get hasAbsolutePath => _path.startsWith('/');
+
+  /**
    * Returns the origin of the URI in the form scheme://host:port for the
    * schemes http and https.
    *