pkg/shelf: case-insensitive headers, cleaner Request ctor, a lot more tests

pkg/shelf: case-insensitive headers, cleaner Request ctor, a lot more tests

R=nweiz@google.com

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

[kevmoo, 2014-04-08 02:21:05]

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.dart
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:17: final Uri uri;
open for discussion on the name here...

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:76: // TODO(kevmoo) finish documenting the rest
of the arguments.
Yes, there needs to be more here -- but this is an improvement over what we had.

[nweiz, 2014-04-08 20:23:08]

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.dart
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:12: import 'package:collection/wrappers.dart' as
pc show UnmodifiableMapView;
There's never a reason to use "show" and "as" in the same import. I prefer "as"
in general.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:98: pc.UnmodifiableMapView
_getIgnoreCaseMapView(Map headers) {
Document this.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:101: hashCode: (key) =>
key.toLowerCase().hashCode);
Add a TODO to use a canonicalized map once something like that exists.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:105: // TODO(kevmoo): use UnmodifiableMapView
from SDK once 1.4 ships
You don't need this TODO twice in the same file.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.dart
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:17: final Uri uri;
On 2014/04/08 02:21:05, kevmoo wrote:
> open for discussion on the name here...

I prefer "url".

Also, this needs a lot more documentation. It should explain everything that's
currently explained by [pathInfo].

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:32: String get queryString => uri.query;
I thought you said you were getting rid of these.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:69: /// Creats a new [Request].
"Creats" -> "Creates"

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:77: Request(this.protocolVersion, this.method,
Map<String, String> headers,
I feel like [protocolVersion] should be named as well, and just default to
"1.1". It's nice to make it as easy as possible to manually construct a
reasonable-looking request.

Also it feels like [requestedUri] should go between [method] and [headers], to
match the order in the actual request. I could see [headers] being optional,
too, since a minimal valid request doesn't include any.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:80: this.uri = _getUri(requestedUri, uri,
scriptName),
Nit: I'd use "compute" rather than "get" here and below, since you're not so
much looking up the data as figuring it out.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:87: throw new ArgumentError('requstedUri must be
an absolute URI');
Period at the end of the error message, here and below.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:123: Uri _getUri(Uri requestedUri, Uri uri,
String scriptName) {
Document these.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:133: 'pathInfo and scriptName must both be null
or both be set');
Nit: It's confusing that you start out using if/else if but then switch to using
short-circuiting for control flow. Either wrap this in an "else" clause or
remove the "else" above. Same below.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/message_test....
File pkg/shelf/test/message_test.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/message_test....
pkg/shelf/test/message_test.dart:16: return new Message(headers, body);
It seems like Message should be an abstract class, right? There's no (non-test)
reason to want to instantiate it and not its base classes.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/message_test....
pkg/shelf/test/message_test.dart:21: var message = _createMessage(headers: {
'foo': 'bar' });
Nit: "{'foo': 'bar'}"

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
File pkg/shelf/test/request_test.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:58: uri: Uri.parse(pathInfo), scriptName:
scriptName);
Testing this with the same data in [requestedUrl] as is in [uri]/[scriptName]
isn't really testing much. The interesting case is which takes precedence when
they differ.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:77: test('bad uri', () {
"bad" isn't very descriptive. What's bad about it? Same below.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:96: pathInfo = '/assets/static/page';
If you're testing multiple possible errors, put them in separate test cases with
names that describe exactly what the errors are.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:121: group("readAsString", () {
I thought the reason you added message_test was so that you wouldn't have
duplicate tests like this one.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/shelf_io_test...
File pkg/shelf/test/shelf_io_test.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/shelf_io_test...
pkg/shelf/test/shelf_io_test.dart:141: 'multi-header': 'foo,bar,baz'
Does dart:io really split comma-separated header values? I thought it considered
multiple headers with the same name as separate.

[kevmoo, 2014-04-08 21:41:13]

PTAL

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.dart
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:12: import 'package:collection/wrappers.dart' as
pc show UnmodifiableMapView;
On 2014/04/08 20:23:09, nweiz wrote:
> There's never a reason to use "show" and "as" in the same import. I prefer
"as"
> in general.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:98: pc.UnmodifiableMapView
_getIgnoreCaseMapView(Map headers) {
On 2014/04/08 20:23:09, nweiz wrote:
> Document this.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:101: hashCode: (key) =>
key.toLowerCase().hashCode);
On 2014/04/08 20:23:09, nweiz wrote:
> Add a TODO to use a canonicalized map once something like that exists.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/message.da...
pkg/shelf/lib/src/message.dart:105: // TODO(kevmoo): use UnmodifiableMapView
from SDK once 1.4 ships
On 2014/04/08 20:23:09, nweiz wrote:
> You don't need this TODO twice in the same file.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.dart
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:17: final Uri uri;
On 2014/04/08 20:23:09, nweiz wrote:
> On 2014/04/08 02:21:05, kevmoo wrote:
> > open for discussion on the name here...
> 
> I prefer "url".
> 
> Also, this needs a lot more documentation. It should explain everything that's
> currently explained by [pathInfo].

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:32: String get queryString => uri.query;
On 2014/04/08 20:23:09, nweiz wrote:
> I thought you said you were getting rid of these.

Kept them around for convenience. Worth while?

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:69: /// Creats a new [Request].
On 2014/04/08 20:23:09, nweiz wrote:
> "Creats" -> "Creates"

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:77: Request(this.protocolVersion, this.method,
Map<String, String> headers,
On 2014/04/08 20:23:09, nweiz wrote:
> I feel like [protocolVersion] should be named as well, and just default to
> "1.1". It's nice to make it as easy as possible to manually construct a
> reasonable-looking request.
> 
> Also it feels like [requestedUri] should go between [method] and [headers], to
> match the order in the actual request. I could see [headers] being optional,
> too, since a minimal valid request doesn't include any.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:80: this.uri = _getUri(requestedUri, uri,
scriptName),
On 2014/04/08 20:23:09, nweiz wrote:
> Nit: I'd use "compute" rather than "get" here and below, since you're not so
> much looking up the data as figuring it out.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:87: throw new ArgumentError('requstedUri must be
an absolute URI');
On 2014/04/08 20:23:09, nweiz wrote:
> Period at the end of the error message, here and below.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:123: Uri _getUri(Uri requestedUri, Uri uri,
String scriptName) {
On 2014/04/08 20:23:09, nweiz wrote:
> Document these.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:133: 'pathInfo and scriptName must both be null
or both be set');
On 2014/04/08 20:23:09, nweiz wrote:
> Nit: It's confusing that you start out using if/else if but then switch to
using
> short-circuiting for control flow. Either wrap this in an "else" clause or
> remove the "else" above. Same below.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/message_test....
File pkg/shelf/test/message_test.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/message_test....
pkg/shelf/test/message_test.dart:16: return new Message(headers, body);
On 2014/04/08 20:23:09, nweiz wrote:
> It seems like Message should be an abstract class, right? There's no
(non-test)
> reason to want to instantiate it and not its base classes.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/message_test....
pkg/shelf/test/message_test.dart:21: var message = _createMessage(headers: {
'foo': 'bar' });
On 2014/04/08 20:23:09, nweiz wrote:
> Nit: "{'foo': 'bar'}"

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
File pkg/shelf/test/request_test.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:58: uri: Uri.parse(pathInfo), scriptName:
scriptName);
On 2014/04/08 20:23:09, nweiz wrote:
> Testing this with the same data in [requestedUrl] as is in [uri]/[scriptName]
> isn't really testing much. The interesting case is which takes precedence when
> they differ.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:77: test('bad uri', () {
On 2014/04/08 20:23:09, nweiz wrote:
> "bad" isn't very descriptive. What's bad about it? Same below.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:96: pathInfo = '/assets/static/page';
On 2014/04/08 20:23:09, nweiz wrote:
> If you're testing multiple possible errors, put them in separate test cases
with
> names that describe exactly what the errors are.

Done.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/request_test....
pkg/shelf/test/request_test.dart:121: group("readAsString", () {
On 2014/04/08 20:23:09, nweiz wrote:
> I thought the reason you added message_test was so that you wouldn't have
> duplicate tests like this one.

Request and Response have different logic for bodies, so thinks that map to
non-Stream body args I kept separate.

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/shelf_io_test...
File pkg/shelf/test/shelf_io_test.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/test/shelf_io_test...
pkg/shelf/test/shelf_io_test.dart:141: 'multi-header': 'foo,bar,baz'
On 2014/04/08 20:23:09, nweiz wrote:
> Does dart:io really split comma-separated header values? I thought it
considered
> multiple headers with the same name as separate.

HttpHeaders stores values in a List. shelf_io does a join to turn them back into
a string. This is just checked to make sure the round-trip is valid.

[kevmoo, 2014-04-08 21:50:25]

PTAL

[nweiz, 2014-04-08 22:52:43]

lgtm

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.dart
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/227563010/diff/1/pkg/shelf/lib/src/request.da...
pkg/shelf/lib/src/request.dart:32: String get queryString => uri.query;
On 2014/04/08 21:41:13, kevmoo wrote:
> On 2014/04/08 20:23:09, nweiz wrote:
> > I thought you said you were getting rid of these.
> 
> Kept them around for convenience. Worth while?

I think they're more confusing than they're worth.

https://codereview.chromium.org/227563010/diff/60001/pkg/shelf/lib/src/reques...
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/227563010/diff/60001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:104: throw new ArgumentError('pathInfo must be
empty or start with "/".');
"pathInfo" -> "url"

https://codereview.chromium.org/227563010/diff/60001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:108: throw new ArgumentError('scriptName and
pathInfo cannot both be empty.');
"pathInfo" -> "url"

[kevmoo, 2014-04-08 23:23:03]

https://codereview.chromium.org/227563010/diff/60001/pkg/shelf/lib/src/reques...
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/227563010/diff/60001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:104: throw new ArgumentError('pathInfo must be
empty or start with "/".');
On 2014/04/08 22:52:44, nweiz wrote:
> "pathInfo" -> "url"

Done.

https://codereview.chromium.org/227563010/diff/60001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:108: throw new ArgumentError('scriptName and
pathInfo cannot both be empty.');
On 2014/04/08 22:52:44, nweiz wrote:
> "pathInfo" -> "url"

Done.

[kevmoo, 2014-04-08 23:23:06]

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