Allow default charsets for HTTPCodecRepository (#269)

* Allow default charsets for HTTPCodecRepository

* Update a test

* Remove type check in form codec as it shoul no longer be needed

CHANGELOG.md

@@ -1,5 +1,9 @@
 # aqueduct changelog
 
+## 2.2.1
+
+- Allow `HTTPCodecRepository.add` to use specify default charset for Content-Type if a request does not specify one.
+
 ## 2.2.0
 
 - The default template created by `aqueduct create` is now mostly empty. Available templates can be listed with `aqueduct create list-templates` and selected with the command-line option `--template`.

lib/src/http/http_codec_repository.dart

@@ -12,8 +12,8 @@ import 'http.dart';
 /// to add mappings in an application's [RequestSink] subclass constructor.
 class HTTPCodecRepository {
   HTTPCodecRepository._() {
-    add(new ContentType("application", "json"), const JsonCodec(), allowCompression: true);
-    add(new ContentType("application", "x-www-form-urlencoded"), const _FormCodec(), allowCompression: true);
+    add(new ContentType("application", "json", charset: "utf-8"), const JsonCodec(), allowCompression: true);
+    add(new ContentType("application", "x-www-form-urlencoded", charset: "utf-8"), const _FormCodec(), allowCompression: true);
     setAllowsCompression(new ContentType("text", "*"), true);
     setAllowsCompression(new ContentType("application", "javascript"), true);
   }
@@ -28,18 +28,18 @@ class HTTPCodecRepository {
   Map<String, Map<String, Codec>> _subtypeCodecs = {};
   Map<String, bool> _primaryTypeCompressionMap = {};
   Map<String, Map<String, bool>> _subtypeCompressionMap = {};
+  Map<String, Map<String, String>> _defaultCharsetMap = {};
 
   /// Adds a custom [codec] for [contentType].
   ///
-  /// The body of a [Response] sent with [contentType] will be transformed by [codec].
+  /// The body of a [Response] sent with [contentType] will be transformed by [codec]. A [Request] with [contentType] Content-Type
+  /// will be decode its [Request.body] with [codec].
   ///
-  /// [codec] may produce a [List<int>] or [String]. If it produces a [String],
-  /// [contentType]'s primary type must be `text`. Specifying a charset for [contentType] has no effect,
-  /// as a [Response] indicates the charset it will use.
+  /// [codec] must produce a [List<int>] (or used chunked conversion to create a `Stream<List<int>>`).
   ///
-  /// [contentType]'s subtype may be `*`; this signifies that matching is only done on the primary content type.
-  /// For example, if [contentType] is `text/*`, then all `text/` (`text/html`, `text/plain`, etc.) content types
-  /// are converted by [codec].
+  /// [contentType]'s subtype may be `*`; all Content-Type's with a matching [ContentType.primaryType] will be
+  /// encoded or decoded by [codec], regardless of [ContentType.subType]. For example, if [contentType] is `text/*`, then all
+  /// `text/` (`text/html`, `text/plain`, etc.) content types are converted by [codec].
   ///
   /// The most specific codec for a content type is chosen when converting an HTTP body. For example, if both `text/*`
   /// and `text/html` have been added through this method, a [Response] with content type `text/html` will select the codec
@@ -49,6 +49,18 @@ class HTTPCodecRepository {
   /// Media types like images and audio files should avoid setting [allowCompression] because they are already compressed.
   ///
   /// A response with a content type not in this instance will be sent unchanged to the HTTP client (and therefore must be [List<int>]
+  ///
+  /// The [ContentType.charset] is not evaluated when selecting the codec for a content type. However, a charset indicates the default
+  /// used when a request's Content-Type header omits a charset. For example, in order to decode JSON data, the request body must first be decoded
+  /// from a list of bytes into a [String]. If a request omits the charset, this first step is would not be applied and the JSON codec would attempt
+  /// to decode a list of bytes instead of a [String] and would fail. Thus, `application/json` is added through the following:
+  ///
+  ///         HTTPCodecRepository.defaultInstance.add(
+  ///           new ContentType("application", "json", charset: "utf-8"), const JsonCodec(), allowsCompression: true);
+  ///
+  /// In the event that a request is sent without a charset, the codec will automatically apply a UTF8 decode step because of this default.
+  ///
+  /// Only use default charsets when the codec must first be decoded into a [String].
   void add(ContentType contentType, Codec codec, {bool allowCompression: true}) {
     if (contentType.subType == "*") {
       _primaryTypeCodecs[contentType.primaryType] = codec;
@@ -62,6 +74,12 @@ class HTTPCodecRepository {
       innerCompress[contentType.subType] = allowCompression;
       _subtypeCompressionMap[contentType.primaryType] = innerCompress;
     }
+
+    if (contentType.charset != null) {
+      var innerCodecs = _defaultCharsetMap[contentType.primaryType] ?? {};
+      innerCodecs[contentType.subType] = contentType.charset;
+      _defaultCharsetMap[contentType.primaryType] = innerCodecs;
+    }
   }
 
   /// Toggles whether HTTP bodies of [contentType] are compressed with GZIP.
@@ -112,6 +130,8 @@ class HTTPCodecRepository {
       charsetCodec = _codecForCharset(contentType.charset);
     } else if (contentType.primaryType == "text" && contentCodec == null) {
       charsetCodec = LATIN1;
+    } else {
+      charsetCodec = _defaultCharsetCodecForType(contentType);
     }
 
 
@@ -137,6 +157,20 @@ class HTTPCodecRepository {
 
     return encoding;
   }
+
+  Codec _defaultCharsetCodecForType(ContentType type) {
+    var inner = _defaultCharsetMap[type.primaryType];
+    if (inner == null) {
+      return null;
+    }
+
+    var encodingName = inner[type.subType] ?? inner["*"];
+    if (encodingName == null) {
+      return null;
+    }
+
+    return Encoding.getByName(encodingName);
+  }
 }
 
 /// Thrown when [HTTPCodecRepository] encounters an exception.
@@ -153,30 +187,23 @@ class _FormCodec extends Codec {
   const _FormCodec();
 
   @override
-  Converter<Map<String, dynamic>, dynamic> get encoder =>
+  Converter<Map<String, dynamic>, String> get encoder =>
       throw new HTTPCodecException("Cannot encode application/x-www-form-urlencoded data. This content type is only available for decoding.");
 
   @override
-  Converter<dynamic, Map<String, dynamic>> get decoder => const _FormDecoder();
+  Converter<String, Map<String, dynamic>> get decoder => const _FormDecoder();
 }
 
 
-class _FormDecoder extends Converter<dynamic, Map<String, dynamic>> {
+class _FormDecoder extends Converter<String, Map<String, dynamic>> {
   // This class may take input as either String or List<int>. If charset is not defined in request,
   // then data is List<int> (from HTTPCodecRepository) and will default to being UTF8 decoded first.
   // Otherwise, if String, the request body has been decoded according to charset already.
 
   const _FormDecoder();
 
   @override
-  Map<String, dynamic> convert(dynamic data) {
-    if (data is! String) {
-      if (data is List<int>) {
-        data = UTF8.decode(data);
-      } else {
-        throw new HTTPCodecException("Invalid data type '${data.runtimeType}' for '_FormDecoder', must be 'List<int>' or 'String'");
-      }
-    }
+  Map<String, dynamic> convert(String data) {
     var parsed = new Uri(query: data);
 
     return parsed.queryParametersAll;
@@ -188,22 +215,15 @@ class _FormDecoder extends Converter<dynamic, Map<String, dynamic>> {
   }
 }
 
-class _FormSink extends ChunkedConversionSink<dynamic> {
+class _FormSink extends ChunkedConversionSink<String> {
   _FormSink(this._outSink);
 
   final _FormDecoder decoder = const _FormDecoder();
   final Sink<Map<String, dynamic>> _outSink;
   final StringBuffer _buffer = new StringBuffer();
 
   @override
-  void add(dynamic data) {
-    if (data is! String) {
-      if (data is List<int>) {
-        data = UTF8.decode(data);
-      } else {
-        throw new HTTPCodecException("Invalid data type '${data.runtimeType}' for '_FormDecoder', must be 'List<int>' or 'String'");
-      }
-    }
+  void add(String data) {
     _buffer.write(data);
   }
 

pubspec.yaml

@@ -1,5 +1,5 @@
 name: aqueduct
-version: 2.2.0
+version: 2.2.1
 description: Server-side framework, including routing, ORM and OAuth 2.0 in a consistent, rigorously tested framework.
 author: stable|kernel <http://stablekernel.com>
 homepage: https://aqueduct.io

test/base/body_decoder_test.dart

@@ -84,6 +84,20 @@ void main() {
       expect(body, [{"a": "val"}]);
     });
 
+    test("Omit charset from known decoder defaults to charset added if exists", () async {
+      var client = new HttpClient();
+      var req = await client.postUrl(Uri.parse("http://localhost:8123"));
+      req.headers.add(HttpHeaders.CONTENT_TYPE, "application/json");
+      req.add(UTF8.encode(JSON.encode({"a": "val"})));
+      req.close().catchError((err) => null);
+
+      request = new Request(await server.first);
+      expect(request.innerRequest.headers.contentType.charset, null);
+
+      var body = await request.body.decodedData;
+      expect(body, [{"a": "val"}]);
+    });
+
     test("application/x-form-url-encoded decoder works on valid form data",
         () async {
       http
@@ -158,7 +172,7 @@ void main() {
       // We'll just use JSON here so we don't have to write a separate codec
       // to test whether or not this content-type gets paired to a codec.
       HTTPCodecRepository.defaultInstance.add(new ContentType("application", "thingy"), const JsonCodec());
-      HTTPCodecRepository.defaultInstance.add(new ContentType("somethingelse", "*"), const JsonCodec());
+      HTTPCodecRepository.defaultInstance.add(new ContentType("somethingelse", "*", charset: "utf-8"), const JsonCodec());
     });
 
     setUp(() async {
@@ -191,6 +205,42 @@ void main() {
       var body = await request.body.decodedData;
       expect(body, [{"key":"value"}]);
     });
+
+    test("Omit charset from added decoder with default charset and match-all subtype", () async {
+      var client = new HttpClient();
+      var req = await client.postUrl(Uri.parse("http://localhost:8123"));
+      req.headers.add(HttpHeaders.CONTENT_TYPE, "somethingelse/foobar");
+      req.add(UTF8.encode(JSON.encode({"a": "val"})));
+      req.close().catchError((err) => null);
+
+      var request = new Request(await server.first);
+      expect(request.innerRequest.headers.contentType.charset, null);
+
+      var body = await request.body.decodedData;
+      expect(body, [{"a": "val"}]);
+    });
+
+    test("Omit charset from added decoder does not add charset decoded if not specified", () async {
+      var client = new HttpClient();
+      var req = await client.postUrl(Uri.parse("http://localhost:8123"));
+      req.headers.add(HttpHeaders.CONTENT_TYPE, "application/thingy");
+      req.add(UTF8.encode(JSON.encode({"a": "val"})));
+      req.close().catchError((err) => null);
+
+      var request = new Request(await server.first);
+      expect(request.innerRequest.headers.contentType.charset, null);
+
+      // The test fails for a different reason in checked vs. unchecked mode.
+      // Tests run in checked mode, but coverage runs in unchecked mode.
+      var data;
+      try {
+        data = await request.body.decodedData;
+      } catch (e) {
+        expect(e, isNotNull);
+      }
+
+      expect(data, isNull);
+    });
   });
 
   group("Casting methods - map", () {

test/base/body_encoder_streaming_test.dart

@@ -55,9 +55,11 @@ void main() {
     });
 
     test("Stream a list of bytes with incorrect content type returns 500", () async {
+      HTTPCodecRepository.defaultInstance.add(new ContentType("application", "silly"), const JsonCodec());
+
       var sc = new StreamController<List<int>>();
       var response = new Response.ok(sc.stream)
-        ..contentType = new ContentType("application", "json");
+        ..contentType = new ContentType("application", "silly");
       server = await bindAndRespondWith(response);
 
       var resultFuture = http.get("http://localhost:8081");