Do "path normalization" when creating a URI.

Do "path normalization" when creating a URI.

Path normaliztion removes '.' and '..' segments from a URI. Such relative references are only intended for URI References, and Uri References are only intended for resolving against a full URI.

We do path normalization on all URIs that have a scheme, authority or an absolute path, and partial normalization on what are really just relative paths. The partial normalization can leave ".." at the start of the path.

The URI reference resolution algorithm doesn't work as expected for a URI ending in "..". Resolving "./foo" wrt. a base of "/a/.." results in  "/a/foo" - this is avoided when the base is path normalized before it's used.

This also fixes the "normalizePath" function which currently removes leading '..' segments, contrary to its documentation. It also makes the function redundant since all URI paths are normalized automatically.

See discussion on http://dartbug.com/23688


Also fix bug in the removeDotSegments function.

R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/848f6c978c7be89ede9db0c990289d5975ea391c

[Lasse Reichstein Nielsen, 2015-07-17 10:34:23]

lrn@google.com changed reviewers:
+ floitsch@google.com

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

LGTM.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:384: bool hasAuthority = host != null;
nit: (host != null)

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:513: bool hasAuthority = host != null;
ditto.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:516: if (scheme.isEmpty && host == null &&
!path.startsWith('/')) {
ditto.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:960: bool hasAuthority = host != null;
we seem to disagree :)
I like the parenthesis.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1032: * Returns an URI where the path has been normalized.
a URI

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1194: // If they are above _UPPER_CASE_Z, the scheme is
already lower-case.
maybe add assert(_UPPER_CASE_Z < _LOWER_CASE_A);

(for documentation purposes)

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1196: bool allLowercase = minChar > _UPPER_CASE_Z;
why not minChar >= _LOWER_CASE_A ?

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1484: /// Make a guess at whether a path contains a `..`
or `.` segments.
segment.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1489: static bool _hasDotSegments(String path) {
_maybeHasDotSegments ?

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1534: if (!_hasDotSegments(path)) return path;  // Ensures
path non-empty below.
If _hasDotSegments return false for the empty string it must say so in the
documentation.

?:
// Also weeds out empty paths.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1552: if (output.isEmpty || output.length == 1 &&
output[0].isEmpty) return "./";
parenthesis

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1643: // Non-RFC 3986 beavior. If both base and reference
are relative
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 15:09:10]

Committed patchset #3 (id:40001) manually as
848f6c978c7be89ede9db0c990289d5975ea391c (presubmit successful).

[kevmoo, 2015-07-17 16:42:53]

On 2015/07/17 15:09:10, Lasse Reichstein Nielsen wrote:
> Committed patchset #3 (id:40001) manually as
> 848f6c978c7be89ede9db0c990289d5975ea391c (presubmit successful).

THANK YOU for updating the changelog! :-)

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

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:384: bool hasAuthority = host != null;
Done.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:516: if (scheme.isEmpty && host == null &&
!path.startsWith('/')) {
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.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:960: bool hasAuthority = host != null;
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.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1032: * Returns an URI where the path has been normalized.
On 2015/07/17 13:03:20, floitsch wrote:
> a URI

Done.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1194: // If they are above _UPPER_CASE_Z, the scheme is
already lower-case.
I'll revert this, I had missed that there are valid non-letter characters before
'A' as well.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1196: bool allLowercase = minChar > _UPPER_CASE_Z;
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.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1484: /// Make a guess at whether a path contains a `..`
or `.` segments.
On 2015/07/17 13:03:20, floitsch wrote:
> segment.

Done.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1489: static bool _hasDotSegments(String path) {
_mayContainDotSegments

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1534: if (!_hasDotSegments(path)) return path;  // Ensures
path non-empty below.
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.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1552: if (output.isEmpty || output.length == 1 &&
output[0].isEmpty) return "./";
Ack, with mixed || and &&, some parentheses can improve reading.

https://codereview.chromium.org/1224263009/diff/1/sdk/lib/core/uri.dart#newco...
sdk/lib/core/uri.dart:1643: // Non-RFC 3986 beavior. If both base and reference
are relative
Not covered, will document.

[kevmoo, 2015-07-21 17:10:58]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2015-07-21 17:10:59]

Please update changelog w/ the three added properties: hasAbsolutePath,
hasEmptyPath, hasScheme