Accept IPv6 addresses in Uri.http and Uri.https, and correctly nest IPv6 addresses in '[' and ']'.

Accept IPv6 addresses in Uri.http and Uri.https, and correctly nest IPv6 addresses in '[' and ']'.

BUG= http://dartbug.com/13130
BUG= http://dartbug.com/13131
BUG= http://dartbug.com/13132
R=floitsch@google.com, sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27381

[floitsch, 2013-09-09 13:32:21]

DBC.
Please add tests.

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

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode241
sdk/lib/core/uri.dart:241: var hostEnd = null;
That's how I would write it.

var hostEnd = hostStart;
if (authority.codeUnitAt(hostStart) == _LEFT_BRACKET) {
  // IPv6 host.
  for (; hostEnd < authority.length; hostEnd++) {
    if (authority.codeUnitAt(hostEnd) == _RIGHT_BRACKET) break;
  }
  if (hostEnd == authority.length) throw...;
  hostEnd++;  // Skip the closing bracket.
  if (hostEnd != authority.length && authority.codeUnitAt(hostEnd) != _COLON) {
    throw "bad authority".
  }
}
// Split host and port.
bool hasPort = false;
for (int i = hostEnd; i < authority.length; i++) {
  if (authority.codeUnitAt(i) == _COLON) {
    hostEnd = i;
    var portString = authority.substring(i + 1);
    if (portString.isNotEmpty) port = int.parse(portString);
  }
}
host = authority.substring(hostStart, hostEnd);
}

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode261
sdk/lib/core/uri.dart:261: hasPort = true;
you set hasPort to true, but if the host is empty you don't actually provide a
port.

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode264
sdk/lib/core/uri.dart:264: if (!host.isEmpty) {
if you just check hostStart != hostEnd you don't need to take the host string
and can take it unconditionally below.

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode266
sdk/lib/core/uri.dart:266: if (portString.isNotEmpty) port =
int.parse(portString);
is this intended behavior? (an authority with ":" but without the port?

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode497
sdk/lib/core/uri.dart:497: if (host.codeUnitAt(0) == _LEFT_BRACKET) return host;
I would prefer a more sophisticated host-parser:
detect bad hosts with ports ("foo:8080") and don't treat them like ipv6.
Detect obviously bad ipv6 addresses (like :::znth:aaaaaa:)

[Anders Johnsen, 2013-09-10 11:13:23]

PTAL, adding more tests and parsing of IPv6/IPv4 addresses to Uri.

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

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode241
sdk/lib/core/uri.dart:241: var hostEnd = null;
On 2013/09/09 13:32:21, floitsch wrote:
> That's how I would write it.
> 
> var hostEnd = hostStart;
> if (authority.codeUnitAt(hostStart) == _LEFT_BRACKET) {
>   // IPv6 host.
>   for (; hostEnd < authority.length; hostEnd++) {
>     if (authority.codeUnitAt(hostEnd) == _RIGHT_BRACKET) break;
>   }
>   if (hostEnd == authority.length) throw...;
>   hostEnd++;  // Skip the closing bracket.
>   if (hostEnd != authority.length && authority.codeUnitAt(hostEnd) != _COLON)
{
>     throw "bad authority".
>   }
> }
> // Split host and port.
> bool hasPort = false;
> for (int i = hostEnd; i < authority.length; i++) {
>   if (authority.codeUnitAt(i) == _COLON) {
>     hostEnd = i;
>     var portString = authority.substring(i + 1);
>     if (portString.isNotEmpty) port = int.parse(portString);
>   }
> }
> host = authority.substring(hostStart, hostEnd);
> }

Done.

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode261
sdk/lib/core/uri.dart:261: hasPort = true;
On 2013/09/09 13:32:21, floitsch wrote:
> you set hasPort to true, but if the host is empty you don't actually provide a
> port.

Done.

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode264
sdk/lib/core/uri.dart:264: if (!host.isEmpty) {
On 2013/09/09 13:32:21, floitsch wrote:
> if you just check hostStart != hostEnd you don't need to take the host string
> and can take it unconditionally below.

Done.

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode266
sdk/lib/core/uri.dart:266: if (portString.isNotEmpty) port =
int.parse(portString);
On 2013/09/09 13:32:21, floitsch wrote:
> is this intended behavior? (an authority with ":" but without the port?

Done.

https://codereview.chromium.org/23904004/diff/1/sdk/lib/core/uri.dart#newcode497
sdk/lib/core/uri.dart:497: if (host.codeUnitAt(0) == _LEFT_BRACKET) return host;
On 2013/09/09 13:32:21, floitsch wrote:
> I would prefer a more sophisticated host-parser:
> detect bad hosts with ports ("foo:8080") and don't treat them like ipv6.
> Detect obviously bad ipv6 addresses (like :::znth:aaaaaa:)

Done.

[floitsch, 2013-09-10 12:11:46]

LGTM.

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

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:266: for (;hostEnd < authority.length; hostEnd++) {
nit: space after ";" ?

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:269: if (portString.isNotEmpty) port =
int.parse(portString);
do we allow: "foo.bar:" as host?
is this valid?
maybe write small comment explaining that this is ok.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:508: buffer.writeCharCode(_LEFT_BRACKET);
just write "[$host]".
This will use a string-buffer on the VM and the more efficient + in dart2js.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1191: * Parse the [host] as a IPv4 address, returning the
address as a list of
as an IPv4

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1194: * If it's unable to parse the address as IPv4, a
`FormatException` will be
If [host] is not a valid IPv4 address representation throws a [FormatException].

(don't use "it", use [class] and don't use future).

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1216: * Parse the [host] as a IPv6 address, returning the
address as a list of
an IPv6

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1219: * If it's unable to parse the address as IPv6, a
`FormatException` will be
ditto.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1221: */
Provide examples.
This would make it also easier to follow the code.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1239: for (int i = 0; i < host.length; i++) {
Add comment that this will parse all segments except potentially the last one
which is handled after the loop.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1252: throw new FormatException('Only one IPv6 wildcart
`::` is allowed');
wildcard

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1264: if (!(segments.last == -1 && partStart ==
host.length)) {
Do the check explicitly:
if (partStart == host.length && segments.lest != -1) {
  // We allow a trailing ":" only if it part of a wildcard.
  throw new FormatException('...');
}
if (partStart == host.length) {
  ...
}

[Søren Gjesse, 2013-09-10 12:18:02]

lgtm

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

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:266: for (;hostEnd < authority.length; hostEnd++) {
Please add space after ;

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1191: * Parse the [host] as a IPv4 address, returning the
address as a list of
In socket we always write "IP version 4 (IPv4)".

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1200: throw new FormatException(
How about making all these FormatExceptions start with either

Illegal IP version 4 address
Illegal IP version 6 address

And then add the reason after?

Illegal IP version 4 address, an IP version 4 address...

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1208: 'Each part must be in the range of `0...255`');
Only two dots in range.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1211: })
Maybe this should be a Uint8List.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1231: 'Each part must be in the range of `0x0...0xFFFF`');
Only two dots in range.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1284: return segments
Wouldn't it be simpler to allocate a list of length 16 and then fill in the
values? Could be a Uint8List.

https://codereview.chromium.org/23904004/diff/5001/tests/corelib/uri_ipv6_tes...
File tests/corelib/uri_ipv6_test.dart (right):

https://codereview.chromium.org/23904004/diff/5001/tests/corelib/uri_ipv6_tes...
tests/corelib/uri_ipv6_test.dart:134: [32, 16, 131, 107, 65, 121, 0, 0, 0, 0, 0,
0, 131, 107, 65, 121] );
Maybe add a test without wildcards both with and without IPv4 in the end. Also
test more and less than 4 hex digigt between :'s.

[floitsch, 2013-09-10 12:23:32]

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

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1211: })
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> Maybe this should be a Uint8List.

Currently not possible. Afaik IE still doesn't have a shim.
But add a TODO.

[Anders Johnsen, 2013-09-11 12:58:31]

PTAL

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

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:266: for (;hostEnd < authority.length; hostEnd++) {
On 2013/09/10 12:11:46, floitsch wrote:
> nit: space after ";" ?

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:266: for (;hostEnd < authority.length; hostEnd++) {
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> Please add space after ;

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:269: if (portString.isNotEmpty) port =
int.parse(portString);
On 2013/09/10 12:11:46, floitsch wrote:
> do we allow: "foo.bar:" as host?
> is this valid?
> maybe write small comment explaining that this is ok.

Yes, we even test for that: 
  
      check(new Uri.http("@host:", ""), "http://host");

Adding comment.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:508: buffer.writeCharCode(_LEFT_BRACKET);
On 2013/09/10 12:11:46, floitsch wrote:
> just write "[$host]".
> This will use a string-buffer on the VM and the more efficient + in dart2js.

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1191: * Parse the [host] as a IPv4 address, returning the
address as a list of
On 2013/09/10 12:11:46, floitsch wrote:
> as an IPv4

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1194: * If it's unable to parse the address as IPv4, a
`FormatException` will be
On 2013/09/10 12:11:46, floitsch wrote:
> If [host] is not a valid IPv4 address representation throws a
[FormatException].
> 
> (don't use "it", use [class] and don't use future).

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1200: throw new FormatException(
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> How about making all these FormatExceptions start with either
> 
> Illegal IP version 4 address
> Illegal IP version 6 address
> 
> And then add the reason after?
> 
> Illegal IP version 4 address, an IP version 4 address...

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1208: 'Each part must be in the range of `0...255`');
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> Only two dots in range.

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1216: * Parse the [host] as a IPv6 address, returning the
address as a list of
On 2013/09/10 12:11:46, floitsch wrote:
> an IPv6

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1219: * If it's unable to parse the address as IPv6, a
`FormatException` will be
On 2013/09/10 12:11:46, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1221: */
On 2013/09/10 12:11:46, floitsch wrote:
> Provide examples.
> This would make it also easier to follow the code.

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1231: 'Each part must be in the range of `0x0...0xFFFF`');
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> Only two dots in range.

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1239: for (int i = 0; i < host.length; i++) {
On 2013/09/10 12:11:46, floitsch wrote:
> Add comment that this will parse all segments except potentially the last one
> which is handled after the loop.

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1252: throw new FormatException('Only one IPv6 wildcart
`::` is allowed');
On 2013/09/10 12:11:46, floitsch wrote:
> wildcard

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1264: if (!(segments.last == -1 && partStart ==
host.length)) {
On 2013/09/10 12:11:46, floitsch wrote:
> Do the check explicitly:
> if (partStart == host.length && segments.lest != -1) {
>   // We allow a trailing ":" only if it part of a wildcard.
>   throw new FormatException('...');
> }
> if (partStart == host.length) {
>   ...
> }

Done.

https://codereview.chromium.org/23904004/diff/5001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1284: return segments
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> Wouldn't it be simpler to allocate a list of length 16 and then fill in the
> values? Could be a Uint8List.

IMO this is easier, than using indexes into a list. Adding Uint8List todo.

https://codereview.chromium.org/23904004/diff/5001/tests/corelib/uri_ipv6_tes...
File tests/corelib/uri_ipv6_test.dart (right):

https://codereview.chromium.org/23904004/diff/5001/tests/corelib/uri_ipv6_tes...
tests/corelib/uri_ipv6_test.dart:134: [32, 16, 131, 107, 65, 121, 0, 0, 0, 0, 0,
0, 131, 107, 65, 121] );
On 2013/09/10 12:18:03, Søren Gjesse wrote:
> Maybe add a test without wildcards both with and without IPv4 in the end. Also
> test more and less than 4 hex digigt between :'s.

Done.

[floitsch, 2013-09-11 13:13:42]

LGTM.

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

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1191: * Parse the [host] as a IP version 4 (IPv4) address,
returning the address
an IP

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1205: // TODO: Consider using Uint8List.
TODO(issue-number) or
TODO(ldap)

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1218: * Parse the [host] as a IP version 6 (IPv6) address,
returning the address
an IP

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1222: * representation.
Still missing the examples in the documentation.

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1300: // TODO: Consider using Uint8List.
ditto.

[Anders Johnsen, 2013-09-11 13:21:43]

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

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1191: * Parse the [host] as a IP version 4 (IPv4) address,
returning the address
On 2013/09/11 13:13:42, floitsch wrote:
> an IP

Done.

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1205: // TODO: Consider using Uint8List.
On 2013/09/11 13:13:42, floitsch wrote:
> TODO(issue-number) or
> TODO(ldap)

Done.

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1218: * Parse the [host] as a IP version 6 (IPv6) address,
returning the address
On 2013/09/11 13:13:42, floitsch wrote:
> an IP

Done.

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1222: * representation.
On 2013/09/11 13:13:42, floitsch wrote:
> Still missing the examples in the documentation.

Done.

https://codereview.chromium.org/23904004/diff/15001/sdk/lib/core/uri.dart#new...
sdk/lib/core/uri.dart:1300: // TODO: Consider using Uint8List.
On 2013/09/11 13:13:42, floitsch wrote:
> ditto.

Done.

[Anders Johnsen, 2013-09-11 13:24:19]

Committed patchset #5 manually as r27381 (presubmit successful).