pkg/shelf: change helper method on Request and Response

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 'package:http_parser/http_parser.dart';
 
@@ skipping 63 matching lines @@
     Stream<List<int>> body, Map<String, Object> context})
       : this.requestedUri = requestedUri,
         this.protocolVersion = protocolVersion == null ?
             '1.1' : protocolVersion,
         this.url = _computeUrl(requestedUri, url, scriptName),
         this.scriptName = _computeScriptName(requestedUri, url, scriptName),
         super(body == null ? new Stream.fromIterable([]) : body,
             headers: headers, context: context) {
     if (method.isEmpty) throw new ArgumentError('method cannot be empty.');
 
-    // TODO(kevmoo) use isAbsolute property on Uri once Issue 18053 is fixed
+    // NOTE: A Uri with a #fragment is is not considered 'absolute' per
+    // http://tools.ietf.org/html/rfc3986#section-4.3
+    // So instead of using Uri.isAbsolute, just checking for an empty scheme

[nweiz, 2014/04/28 22:13:33]

Use full sentences in comments.

[kevmoo, 2014/04/29 10:41:54]

Done.

     if (requestedUri.scheme.isEmpty) {

[kevmoo, 2014/04/26 16:34:34]

The RFC is a bit weird about fragments, hence Uri.isAbsolute is weird.

Just checking for a scheme instead.

[nweiz, 2014/04/28 22:13:33]

I think we should use [isAbsolute] here. The requested URI should never include
a fragment identifier anyway.

[kevmoo, 2014/04/29 10:41:54]

I double checked. dart_io doesn't send the fragment. Done.

       throw new ArgumentError('requstedUri must be an absolute URI.');
     }
 
     if (this.scriptName.isNotEmpty && !this.scriptName.startsWith('/')) {
       throw new ArgumentError('scriptName must be empty or start with "/".');
     }
 
     if (this.scriptName == '/') {
       throw new ArgumentError(
           'scriptName can never be "/". It should be empty instead.');
     }
 
     if (this.scriptName.endsWith('/')) {
       throw new ArgumentError('scriptName must not end with "/".');
     }
 
     if (this.url.path.isNotEmpty && !this.url.path.startsWith('/')) {
       throw new ArgumentError('url must be empty or start with "/".');
     }
 
     if (this.scriptName.isEmpty && this.url.path.isEmpty) {
       throw new ArgumentError('scriptName and url cannot both be empty.');
     }
   }
+
+  /// Copies the request updated [context] and [headers] values.

[nweiz, 2014/04/28 22:13:33]

Fix this sentence.

[kevmoo, 2014/04/29 10:41:54]

Done.

+  ///
+  /// New key-value pairs in [context] and [headers] will be added to the copied
+  /// [Request].
+  ///
+  /// If [context] or [headers] includes a key that already exists, the
+  /// key-value pair will replace the corresponding entry in the copied
+  /// [Request].
+  ///
+  /// All other context and header values from the [Request] will be included
+  /// in the copied [Request] unchanged.
+  Request copy({Map<String, String> headers, Map<String, Object> context}) {

[nweiz, 2014/04/28 22:13:33]

We should discuss this name a bit. I like "copy" okay, but it implies that the
primary purpose of this method is to make a copy of request. For MediaType I
used "change", which is a little longer but communicates that this is the way
you change the fields of the request.

Also, this should be able to change any properties exposed by the request,
although it's fine to do that in a followup CL.

[kevmoo, 2014/04/29 10:41:54]

Done.

+    headers = _updatedMap(this.headers, headers);
+    context = _updatedMap(this.context, context);
+
+    return new Request(this.method, this.requestedUri,
+        protocolVersion: this.protocolVersion, headers: headers, url: this.url,
+        scriptName: this.scriptName, body: this.read(), context: context);

[nweiz, 2014/04/28 22:13:33]

I don't like that this copies the headers and re-wraps the context map even when
they're not modified at all. I suggest you create a _ShelfUnmodifiableMapView
that extends UnmodifiableMapView, use that to wrap [headers] and [context], and
check for it in the Message constructor to see if you can avoid re-copying
and/or re-wrapping.

[kevmoo, 2014/04/29 10:41:54]

Done.

+  }
+}
+
+Map _updatedMap(Map original, Map updates) {

[nweiz, 2014/04/28 22:13:33]

Functions should have verb names, e.g. "_updateMap". Also, document this.

[kevmoo, 2014/04/29 10:41:54]

Done.

+  if (updates == null || updates.isEmpty) return original;
+
+  return new Map.from(original)
+      ..addAll(updates);
 }
 
 /// Computes `url` from the provided [Request] constructor arguments.
 ///
 /// If [url] and [scriptName] are `null`, infer value from [requestedUrl],
 /// otherwise return [url].
 ///
 /// If [url] is provided, but [scriptName] is omitted, throws an
 /// [ArgumentError].
 Uri _computeUrl(Uri requestedUri, Uri url, String scriptName) {
@@ skipping 24 matching lines @@
     return '';
   }
 
   if (url != null && scriptName != null) {
     return scriptName;
   }
 
   throw new ArgumentError(
       'url and scriptName must both be null or both be set.');
 }