pkg/shelf: change helper method on Request and Response

pkg/shelf/lib/src/message.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.message;
 
 import 'dart:async';
 import 'dart:collection';
 import 'dart:convert';
 
@@ skipping 25 matching lines @@
   /// The streaming body of the message.
   ///
   /// This can be read via [read] or [readAsString].
   final Stream<List<int>> _body;
 
   /// Creates a new [Message].
   ///
   /// If [headers] is `null`, it is treated as empty.
   Message(this._body, {Map<String, String> headers,
         Map<String, Object> context})
-      : this.headers = _getIgnoreCaseMapView(headers),
-        this.context = new pc.UnmodifiableMapView(
-            context == null ? const {} : new Map.from(context));
+      : this.headers = new _ShelfUnmodifiableMap<String>(headers, true),
+        this.context = new _ShelfUnmodifiableMap<Object>(context, false);
 
   /// The contents of the content-length field in [headers].
   ///
   /// If not set, `null`.
   int get contentLength {
     if (_contentLengthCache != null) return _contentLengthCache;
     if (!headers.containsKey('content-length')) return null;
     _contentLengthCache = int.parse(headers['content-length']);
     return _contentLengthCache;
   }
@@ skipping 46 matching lines @@
   /// If [encoding] is passed, that's used to decode the body.
   /// Otherwise the encoding is taken from the Content-Type header. If that
   /// doesn't exist or doesn't have a "charset" parameter, UTF-8 is used.
   ///
   /// This calls [read] internally, which can only be called once.
   Future<String> readAsString([Encoding encoding]) {
     if (encoding == null) encoding = this.encoding;
     if (encoding == null) encoding = UTF8;
     return Chain.track(encoding.decodeStream(read()));
   }
+
+  /// Creates a new [Message] by copying existing values and applying specified
+  /// changes.
+  Message change({Map<String, String> headers, Map<String, Object> context});
 }
 
-/// Creates on an unmodifiable map of [headers] with case-insensitive access.
-Map<String, String> _getIgnoreCaseMapView(Map<String, String> headers) {
-  if (headers == null) return const {};
-  // TODO(kevmoo) generalize this model with a 'canonical map' to align with
-  // similiar implementation in http pkg [BaseRequest].
-  var map = new LinkedHashMap<String, String>(
-      equals: (key1, key2) => key1.toLowerCase() == key2.toLowerCase(),
-      hashCode: (key) => key.toLowerCase().hashCode);
+/// Returns a [Map] with the values from [original] and the values from
+/// [updates].
+///
+/// For keys that are the same between [original] and [updates], the value in
+/// [updates] is used.
+///
+/// If [updates] is `null` or empty, [original] is returned unchanged.
+Map updateMap(Map original, Map updates) {

[nweiz, 2014/04/29 19:48:48]

Put this in util.

[kevmoo, 2014/04/30 14:27:52]

Neither Request nor Response import util at the moment.

But they both import Message. Seems okay to keep msg-specific helpers here IMHO

[nweiz, 2014/04/30 20:21:39]

It's not onerous to add an import statement.
This isn't message-specific. It's a helper that non-destructively adds elements
from one map to another. That's pretty general functionality, and so belongs in
util.

[kevmoo, 2014/05/02 16:18:01]

*grumble* Done *eye roll*

+  if (updates == null || updates.isEmpty) return original;
 
-  map.addAll(headers);
+  return new Map.from(original)
+      ..addAll(updates);
+}
 
-  return new pc.UnmodifiableMapView<String, String>(map);
+/// A simple wrapper over [pc.UnmodifiableMapView] which avoids re-wrapping
+/// itself.
+///
+/// If [source] is a [_ShelfUnmodifiableMap] with matcher [ignoreKeyCase], then

[nweiz, 2014/04/29 19:48:48]

"matcher" -> "matching"

This and the following text should be on the constructor, since they're about
how constructor arguments are handled.

[kevmoo, 2014/04/30 14:27:52]

Done.

+/// [source] is returned.
+///
+/// If [source] is `null` it is treated like an empty map.
+///
+/// If [ignoreKeyCase] is `true`, the keys will have case-insensitive access.
+///
+/// [source] is copied to a new [Map] to ensure changes to the paramater value
+/// after constructions are not reflected.
+class _ShelfUnmodifiableMap<V> extends pc.UnmodifiableMapView<String, V> {

[nweiz, 2014/04/29 19:48:48]

This should probably go in its own library.

[kevmoo, 2014/04/30 14:27:52]

I like having it private here. It's purely an implementation detail of Message.
No possible way for someone to import it via pkg uri silly.

[nweiz, 2014/04/30 20:21:39]

Shoving multiple classes into a single library when their implementations aren't
closely tied to one another tends to produce large and unfocused files. The
notion of an immutable map with an optimized constructor isn't related to the
notion of a message at all.
Regardless of whether it's enforced by the language, external users aren't
allowed to import other packages' src/ libraries. Saying "but users could import
it" is by no means a good reason not to use multiple libraries in a package's
implementation.

[kevmoo, 2014/05/02 16:18:01]

fine fine Done yeah yeah

+  final bool ignoreKeyCase;

[nweiz, 2014/04/29 19:48:48]

Document this field.

[kevmoo, 2014/04/30 14:27:52]

Done.

+
+  factory _ShelfUnmodifiableMap(Map<String, V> source, bool ignoreKeyCase) {

[nweiz, 2014/04/29 19:48:48]

Make [ignoreKeyCase] a named argument so it's clearer at the call site.

[kevmoo, 2014/04/30 14:27:52]

Done.

+    if (source is _ShelfUnmodifiableMap<V> &&
+        source.ignoreKeyCase == ignoreKeyCase) {
+      return source;
+    }
+
+    if (source == null || source.isEmpty) {
+      return new _ShelfUnmodifiableMap<V>._(const {}, ignoreKeyCase);

[nweiz, 2014/04/29 19:48:48]

It would be really nice to be able to just return "const {}" here so we can
avoid any extra object allocations for the common case of an empty context map.
Maybe instead of a factory constructor, this should be a static method?

    static Map<String, dynamic> wrap(Map<String, dynamic> source, {bool
ignoreKeyCase});

[kevmoo, 2014/04/30 14:27:52]

Take a look at my alternative. We keep the generic types and get nice const
behavior

+    }
+
+    if (ignoreKeyCase) {
+      // TODO(kevmoo) generalize this model with a 'canonical map' to align with
+      // similiar implementation in http pkg [BaseRequest].
+      var map = new LinkedHashMap<String, V>(
+          equals: (key1, key2) => key1.toLowerCase() == key2.toLowerCase(),
+          hashCode: (key) => key.toLowerCase().hashCode);
+
+      map.addAll(source);
+
+      source = map;
+    } else {
+      source = new Map<String, V>.from(source);
+    }
+
+    return new _ShelfUnmodifiableMap<V>._(source, ignoreKeyCase);
+  }
+
+  _ShelfUnmodifiableMap._(Map<String, V> source, this.ignoreKeyCase)
+      : super(source);
 }