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

pkg/shelf/lib/src/request.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library shelf.request;
 
 import 'dart:async';
-import 'dart:collection';
 
-// TODO(kevmoo): use UnmodifiableMapView from SDK once 1.4 ships
-import 'package:collection/wrappers.dart' as pc;
 import 'package:http_parser/http_parser.dart';
 import 'package:path/path.dart' as p;
 
 import 'message.dart';
 
 /// Represents an HTTP request to be processed by a Shelf application.
 class Request extends Message {
+  /// Represents the path, query string, and fragment of the request.
+  final Uri uri;

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

open for discussion on the name here...

[nweiz, 2014/04/08 20:23:09]

I prefer "url".

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

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

Done.

+
   /// The remainder of the [requestedUri] path designating the virtual
   /// "location" of the request's target within the handler.
   ///
   /// [pathInfo] may be an empty string, if [requestedUri ]targets the handler
   /// root and does not have a trailing slash.
   ///
   /// [pathInfo] is never null. If it is not empty, it will start with `/`.
   ///
   /// [scriptName] and [pathInfo] combine to create a valid path that should
   /// correspond to the [requestedUri] path.
-  final String pathInfo;
+  String get pathInfo => uri.path;
 
   /// The portion of the request URL that follows the "?", if any.
-  final String queryString;
+  String get queryString => uri.query;

[nweiz, 2014/04/08 20:23:09]

I thought you said you were getting rid of these.

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

Kept them around for convenience. Worth while?

[nweiz, 2014/04/08 22:52:44]

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

 
   /// The HTTP request method, such as "GET" or "POST".
   final String method;
 
   /// The initial portion of the [requestedUri] path that corresponds to the
   /// handler.
   ///
   /// [scriptName] allows a handler to know its virtual "location".
   ///
   /// If the handler corresponds to the "root" of a server, it will be an
@@ skipping 16 matching lines @@
   /// This is parsed from the If-Modified-Since header in [headers]. If
   /// [headers] doesn't have an If-Modified-Since header, this will be `null`.
   DateTime get ifModifiedSince {
     if (_ifModifiedSinceCache != null) return _ifModifiedSinceCache;
     if (!headers.containsKey('if-modified-since')) return null;
     _ifModifiedSinceCache = parseHttpDate(headers['if-modified-since']);
     return _ifModifiedSinceCache;
   }
   DateTime _ifModifiedSinceCache;
 
-  Request(this.pathInfo, String queryString, this.method,
-      this.scriptName, this.protocolVersion, this.requestedUri,
-      Map<String, String> headers, {Stream<List<int>> body})
-      : this.queryString = queryString == null ? '' : queryString,
-        super(new pc.UnmodifiableMapView(new HashMap.from(headers)),
-            body == null ? new Stream.fromIterable([]) : body) {
+  /// Creats a new [Request].

[nweiz, 2014/04/08 20:23:09]

"Creats" -> "Creates"

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

Done.

+  ///
+  /// If [uri] and [scriptName] are omitted, they are inferred from
+  /// [requestedUri].
+  ///
+  /// Setting one of [uri] or [scriptName] and not the other will throw an
+  /// [ArgumentError].
+  // TODO(kevmoo) finish documenting the rest of the arguments.

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

Yes, there needs to be more here -- but this is an improvement over what we had.

+  Request(this.protocolVersion, this.method, Map<String, String> headers,

[nweiz, 2014/04/08 20:23:09]

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.

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

Done.

+      Uri requestedUri, {Uri uri, String scriptName, Stream<List<int>> body})
+      : this.requestedUri = requestedUri,
+        this.uri = _getUri(requestedUri, uri, scriptName),

[nweiz, 2014/04/08 20:23:09]

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.

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

Done.

+        this.scriptName = _getScriptName(requestedUri, uri, scriptName),
+        super(headers, body == null ? new Stream.fromIterable([]) : body) {
     if (method.isEmpty) throw new ArgumentError('method cannot be empty.');
 
-    if (scriptName.isNotEmpty && !scriptName.startsWith('/')) {
+    // TODO(kevmoo) use isAbsolute property on Uri once Issue 18053 is fixed
+    if (requestedUri.scheme.isEmpty) {
+      throw new ArgumentError('requstedUri must be an absolute URI');

[nweiz, 2014/04/08 20:23:09]

Period at the end of the error message, here and below.

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

Done.

+    }
+
+    if (this.scriptName.isNotEmpty && !this.scriptName.startsWith('/')) {
       throw new ArgumentError('scriptName must be empty or start with "/".');
     }
 
-    if (scriptName == '/') {
+    if (this.scriptName == '/') {
       throw new ArgumentError(
           'scriptName can never be "/". It should be empty instead.');
     }
 
-    if (scriptName.endsWith('/')) {
+    if (this.scriptName.endsWith('/')) {
       throw new ArgumentError('scriptName must not end with "/".');
     }
 
-    if (pathInfo.isNotEmpty && !pathInfo.startsWith('/')) {
+    if (this.pathInfo.isNotEmpty && !this.pathInfo.startsWith('/')) {
       throw new ArgumentError('pathInfo must be empty or start with "/".');
     }
 
-    if (scriptName.isEmpty && pathInfo.isEmpty) {
+    if (this.scriptName.isEmpty && this.pathInfo.isEmpty) {
       throw new ArgumentError('scriptName and pathInfo cannot both be empty.');
     }
   }
 
   /// Convenience property to access [pathInfo] data as a [List].
   List<String> get pathSegments {
     var segs = p.url.split(pathInfo);
     if (segs.length > 0) {
       assert(segs.first == p.url.separator);
       segs.removeAt(0);
     }
     return segs;
   }
 }
+
+Uri _getUri(Uri requestedUri, Uri uri, String scriptName) {

[nweiz, 2014/04/08 20:23:09]

Document these.

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

Done.

+  if (uri == null && scriptName == null) {
+    return new Uri(path: requestedUri.path, query: requestedUri.query,
+        fragment: requestedUri.fragment);
+  } else if (uri != null && scriptName != null) {
+    // TODO(kevmoo) use isAbsolute property on Uri once Issue 18053 is fixed
+    if (uri.scheme.isNotEmpty) throw new ArgumentError('uri must be relative');
+    return uri;
+  }
+  throw new ArgumentError(
+      'pathInfo and scriptName must both be null or both be set');

[nweiz, 2014/04/08 20:23:09]

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.

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

Done.

+}
+
+String _getScriptName(Uri requstedUri, Uri pathInfo, String scriptName) {
+  if (pathInfo == null && scriptName == null) {
+    return '';
+  } else if (pathInfo != null && scriptName != null) {
+    return scriptName;
+  }
+  throw new ArgumentError(
+      'pathInfo and scriptName must both be null or both be set');
+}