From ff47888a0d727234985c3d7954b808882e6b0f74 Mon Sep 17 00:00:00 2001 From: "sgjesse@google.com" Date: Thu, 7 Mar 2013 07:25:56 +0000 Subject: [PATCH] Make instances of HeaderValue and ContentType immutable Their parameters map is still mutable though. R=ajohnsen@google.com BUG= Review URL: https://codereview.chromium.org//12440002 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@19598 260f80e4-7a28-3924-810f-c04153c831b5 --- pkg/http/lib/src/multipart_file.dart | 11 +-- pkg/http/lib/src/multipart_request.dart | 4 +- pkg/http/lib/src/request.dart | 17 +++-- pkg/http/test/http_test.dart | 4 +- pkg/http/test/multipart_test.dart | 12 ++-- pkg/http/test/request_test.dart | 16 ++--- sdk/lib/io/http.dart | 46 +++++++------ sdk/lib/io/http_headers.dart | 74 ++++++++++----------- sdk/lib/io/string_transformer.dart | 17 +++-- tests/standalone/io/http_advanced_test.dart | 9 +-- tests/standalone/io/http_headers_test.dart | 53 +++++++++------ 11 files changed, 148 insertions(+), 115 deletions(-) diff --git a/pkg/http/lib/src/multipart_file.dart b/pkg/http/lib/src/multipart_file.dart index aa30fc35098..2f1f10268ee 100644 --- a/pkg/http/lib/src/multipart_file.dart +++ b/pkg/http/lib/src/multipart_file.dart @@ -67,12 +67,15 @@ class MultipartFile { /// the future may be inferred from [filename]. factory MultipartFile.fromString(String field, String value, {String filename, ContentType contentType}) { - contentType = contentType == null ? new ContentType("text", "plain") : - // Make a copy of the original contentType so we can modify charset. - new ContentType.fromString(contentType.toString()); + contentType = contentType == null ? new ContentType("text", "plain") + : contentType; var charset = contentType.charset; var encoding = encodingForCharset(contentType.charset, Encoding.UTF_8); - contentType.charset = encoding.name; + // Make a new contentType with ensured charset. + contentType = new ContentType(contentType.primaryType, + contentType.subType, + charset: encoding.name, + parameters: contentType.parameters); return new MultipartFile.fromBytes(field, encodeString(value, encoding), filename: filename, diff --git a/pkg/http/lib/src/multipart_request.dart b/pkg/http/lib/src/multipart_request.dart index 0855063b1fa..bc588713cf6 100644 --- a/pkg/http/lib/src/multipart_request.dart +++ b/pkg/http/lib/src/multipart_request.dart @@ -1,4 +1,4 @@ -// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file +// Copyright (c) 2013, 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. @@ -144,7 +144,7 @@ class MultipartRequest extends BaseRequest { // URL-encode them so we do the same. var header = 'content-disposition: form-data; name="${encodeUri(name)}"'; if (!isPlainAscii(value)) { - header = '$header\r\ncontent-type: text/plain; charset=UTF-8'; + header = '$header\r\ncontent-type: text/plain; charset=utf-8'; } return '$header\r\n\r\n'; } diff --git a/pkg/http/lib/src/request.dart b/pkg/http/lib/src/request.dart index 670bc483722..525f1aed6e5 100644 --- a/pkg/http/lib/src/request.dart +++ b/pkg/http/lib/src/request.dart @@ -1,4 +1,4 @@ -// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file +// Copyright (c) 2013, 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. @@ -57,7 +57,10 @@ class Request extends BaseRequest { _defaultEncoding = value; var contentType = _contentType; if (contentType != null) { - contentType.charset = value.name; + contentType = new ContentType(contentType.primaryType, + contentType.subType, + charset: value.name, + parameters: contentType.parameters); _contentType = contentType; } } @@ -87,8 +90,14 @@ class Request extends BaseRequest { set body(String value) { bodyBytes = encodeString(value, encoding); var contentType = _contentType; - if (contentType == null) contentType = new ContentType("text", "plain"); - if (contentType.charset == null) contentType.charset = encoding.name; + if (contentType == null) { + contentType = new ContentType("text", "plain", charset: encoding.name); + } else if (contentType.charset == null) { + contentType = new ContentType(contentType.primaryType, + contentType.subType, + charset: encoding.name, + parameters: contentType.parameters); + } _contentType = contentType; } diff --git a/pkg/http/test/http_test.dart b/pkg/http/test/http_test.dart index 1fc7f573afa..2dd5d3e4fc8 100644 --- a/pkg/http/test/http_test.dart +++ b/pkg/http/test/http_test.dart @@ -58,7 +58,7 @@ main() { 'path': '/', 'headers': { 'content-type': [ - 'application/x-www-form-urlencoded; charset=UTF-8' + 'application/x-www-form-urlencoded; charset=utf-8' ], 'content-length': ['40'], 'x-random-header': ['Value'], @@ -107,7 +107,7 @@ main() { 'path': '/', 'headers': { 'content-type': [ - 'application/x-www-form-urlencoded; charset=UTF-8' + 'application/x-www-form-urlencoded; charset=utf-8' ], 'content-length': ['40'], 'x-random-header': ['Value'], diff --git a/pkg/http/test/multipart_test.dart b/pkg/http/test/multipart_test.dart index c3c3dd9ddc9..b8210d2fbeb 100644 --- a/pkg/http/test/multipart_test.dart +++ b/pkg/http/test/multipart_test.dart @@ -1,4 +1,4 @@ -// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file +// Copyright (c) 2013, 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. @@ -76,12 +76,12 @@ void main() { value2 --{{boundary}} - content-type: text/plain; charset=UTF-8 + content-type: text/plain; charset=utf-8 content-disposition: form-data; name="file1"; filename="filename1.txt" contents1 --{{boundary}} - content-type: text/plain; charset=UTF-8 + content-type: text/plain; charset=utf-8 content-disposition: form-data; name="file2" contents2 @@ -109,7 +109,7 @@ void main() { expect(request, bodyMatches(''' --{{boundary}} content-disposition: form-data; name="field" - content-type: text/plain; charset=UTF-8 + content-type: text/plain; charset=utf-8 vⱥlūe --{{boundary}}-- @@ -123,7 +123,7 @@ void main() { expect(request, bodyMatches(''' --{{boundary}} - content-type: text/plain; charset=UTF-8 + content-type: text/plain; charset=utf-8 content-disposition: form-data; name="file"; filename="f%C3%AFl%C4%93name.txt" contents @@ -139,7 +139,7 @@ void main() { expect(request, bodyMatches(''' --{{boundary}} - content-type: application/json; charset=UTF-8 + content-type: application/json; charset=utf-8 content-disposition: form-data; name="file" {"hello": "world"} diff --git a/pkg/http/test/request_test.dart b/pkg/http/test/request_test.dart index 551f694bde3..1ef723045eb 100644 --- a/pkg/http/test/request_test.dart +++ b/pkg/http/test/request_test.dart @@ -1,4 +1,4 @@ -// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file +// Copyright (c) 2013, 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. @@ -25,7 +25,7 @@ void main() { 'method': 'POST', 'path': '/', 'headers': { - 'content-type': ['text/plain; charset=UTF-8'], + 'content-type': ['text/plain; charset=utf-8'], 'content-length': ['5'] }, 'body': 'hello' @@ -246,7 +246,7 @@ void main() { var request = new http.Request('POST', dummyUrl); request.bodyFields = {'hello': 'world'}; expect(request.headers[HttpHeaders.CONTENT_TYPE], - equals('application/x-www-form-urlencoded; charset=UTF-8')); + equals('application/x-www-form-urlencoded; charset=utf-8')); }); test('is set to application/x-www-form-urlencoded with the given charset ' @@ -255,7 +255,7 @@ void main() { request.encoding = Encoding.ISO_8859_1; request.bodyFields = {'hello': 'world'}; expect(request.headers[HttpHeaders.CONTENT_TYPE], - equals('application/x-www-form-urlencoded; charset=ISO-8859-1')); + equals('application/x-www-form-urlencoded; charset=iso-8859-1')); }); test('is set to text/plain and the given encoding if body and encoding are ' @@ -264,7 +264,7 @@ void main() { request.encoding = Encoding.ISO_8859_1; request.body = 'hello, world'; expect(request.headers[HttpHeaders.CONTENT_TYPE], - equals('text/plain; charset=ISO-8859-1')); + equals('text/plain; charset=iso-8859-1')); }); test('is modified to include utf-8 if body is set', () { @@ -272,7 +272,7 @@ void main() { request.headers[HttpHeaders.CONTENT_TYPE] = 'application/json'; request.body = '{"hello": "world"}'; expect(request.headers[HttpHeaders.CONTENT_TYPE], - equals('application/json; charset=UTF-8')); + equals('application/json; charset=utf-8')); }); test('is modified to include the given encoding if encoding is set', () { @@ -280,7 +280,7 @@ void main() { request.headers[HttpHeaders.CONTENT_TYPE] = 'application/json'; request.encoding = Encoding.ISO_8859_1; expect(request.headers[HttpHeaders.CONTENT_TYPE], - equals('application/json; charset=ISO-8859-1')); + equals('application/json; charset=iso-8859-1')); }); test('has its charset overridden by an explicit encoding', () { @@ -289,7 +289,7 @@ void main() { 'application/json; charset=utf-8'; request.encoding = Encoding.ISO_8859_1; expect(request.headers[HttpHeaders.CONTENT_TYPE], - equals('application/json; charset=ISO-8859-1')); + equals('application/json; charset=iso-8859-1')); }); test("doen't have its charset overridden by setting bodyFields", () { diff --git a/sdk/lib/io/http.dart b/sdk/lib/io/http.dart index feaeb0e4edb..2eddf7ca202 100644 --- a/sdk/lib/io/http.dart +++ b/sdk/lib/io/http.dart @@ -399,9 +399,7 @@ abstract class HttpHeaders { * use code like this: * * HttpClientRequest request = ...; - * var v = new HeaderValue(); - * v.value = "text/plain"; - * v.parameters["q"] = "0.3" + * var v = new HeaderValue("text/plain", {"q": "0.3"}); * request.headers.add(HttpHeaders.ACCEPT, v); * request.headers.add(HttpHeaders.ACCEPT, "text/html"); * @@ -413,12 +411,16 @@ abstract class HttpHeaders { * HeaderValue v = new HeaderValue.fromString(value); * // Use v.value and v.parameters * }); + * + * An instance of [HeaderValue] is immutable. */ abstract class HeaderValue { /** * Creates a new header value object setting the value part. */ - factory HeaderValue([String value = ""]) => new _HeaderValue(value); + factory HeaderValue([String value = "", Map parameters]) { + return new _HeaderValue(value, parameters); + } /** * Creates a new header value object from parsing a header value @@ -431,9 +433,9 @@ abstract class HeaderValue { } /** - * Gets and sets the header value. + * Gets the header value. */ - String value; + String get value; /** * Gets the map of parameters. @@ -473,15 +475,22 @@ abstract class HttpSession implements Map { /** - * Representation of a content type. + * Representation of a content type. An instance of [ContentType] is + * immutable. */ abstract class ContentType implements HeaderValue { /** * Creates a new content type object setting the primary type and - * sub type. + * sub type. The charset and additional parameters can also be set + * using [charset] and [parameters]. If charset is passed and + * [parameters] contains charset as well the passed [charset] will + * override the value in parameters. Keys and values passed in + * parameters will be converted to lower case. */ - factory ContentType([String primaryType = "", String subType = ""]) { - return new _ContentType(primaryType, subType); + factory ContentType(String primaryType, + String subType, + {String charset, Map parameters}) { + return new _ContentType(primaryType, subType, charset, parameters); } /** @@ -500,24 +509,19 @@ abstract class ContentType implements HeaderValue { } /** - * Gets and sets the content type in the form "primaryType/subType". + * Gets the primary type. */ - String value; + String get primaryType; /** - * Gets and sets the primary type. + * Gets the sub type. */ - String primaryType; + String get subType; /** - * Gets and sets the sub type. + * Gets the character set. */ - String subType; - - /** - * Gets and sets the character set. - */ - String charset; + String get charset; } diff --git a/sdk/lib/io/http_headers.dart b/sdk/lib/io/http_headers.dart index e38ca759ab3..b2b522b3b3e 100644 --- a/sdk/lib/io/http_headers.dart +++ b/sdk/lib/io/http_headers.dart @@ -198,7 +198,7 @@ class _HttpHeaders implements HttpHeaders { if (values != null) { return new ContentType.fromString(values[0]); } else { - return new ContentType(); + return null; } } @@ -277,7 +277,7 @@ class _HttpHeaders implements HttpHeaders { } else if (lowerCaseName == HttpHeaders.CONTENT_TYPE) { _set(HttpHeaders.CONTENT_TYPE, value); } else { - _addValue(lowerCaseName, value); + _addValue(lowerCaseName, value); } } @@ -493,13 +493,18 @@ class _HttpHeaders implements HttpHeaders { class _HeaderValue implements HeaderValue { - _HeaderValue([String this.value = ""]); + String _value; + Map _parameters; - _HeaderValue.fromString(String value, {this.parameterSeparator: ";"}) { + _HeaderValue([String this._value = "", this._parameters]); + + _HeaderValue.fromString(String value, {parameterSeparator: ";"}) { // Parse the string. - _parse(value); + _parse(value, parameterSeparator); } + String get value => _value; + Map get parameters { if (_parameters == null) _parameters = new Map(); return _parameters; @@ -507,7 +512,7 @@ class _HeaderValue implements HeaderValue { String toString() { StringBuffer sb = new StringBuffer(); - sb.write(value); + sb.write(_value); if (parameters != null && parameters.length > 0) { _parameters.forEach((String name, String value) { sb.write("; "); @@ -519,7 +524,7 @@ class _HeaderValue implements HeaderValue { return sb.toString(); } - void _parse(String s) { + void _parse(String s, String parameterSeparator) { int index = 0; bool done() => index == s.length; @@ -606,56 +611,51 @@ class _HeaderValue implements HeaderValue { } skipWS(); - value = parseValue(); + _value = parseValue(); skipWS(); if (done()) return; maybeExpect(parameterSeparator); parseParameters(); } - - String value; - String parameterSeparator; - Map _parameters; } class _ContentType extends _HeaderValue implements ContentType { - _ContentType(String primaryType, String subType) - : _primaryType = primaryType, _subType = subType, super(""); + _ContentType(String primaryType, + String subType, + String charset, + Map parameters) + : _primaryType = primaryType, _subType = subType, super("") { + if (_primaryType == null) _primaryType = ""; + if (_subType == null) _subType = ""; + _value = "$_primaryType/$_subType";; + if (parameters != null) { + parameters.forEach((String key, String value) { + this.parameters[key.toLowerCase()] = value.toLowerCase(); + }); + } + if (charset != null) { + this.parameters["charset"] = charset.toLowerCase(); + } + } - _ContentType.fromString(String value) : super.fromString(value); - - String get value => "$_primaryType/$_subType"; - - void set value(String s) { - int index = s.indexOf("/"); - if (index == -1 || index == (s.length - 1)) { - primaryType = s.trim().toLowerCase(); - subType = ""; + _ContentType.fromString(String value) : super.fromString(value) { + int index = _value.indexOf("/"); + if (index == -1 || index == (_value.length - 1)) { + _primaryType = _value.trim().toLowerCase(); + _subType = ""; } else { - primaryType = s.substring(0, index).trim().toLowerCase(); - subType = s.substring(index + 1).trim().toLowerCase(); + _primaryType = _value.substring(0, index).trim().toLowerCase(); + _subType = _value.substring(index + 1).trim().toLowerCase(); } } String get primaryType => _primaryType; - void set primaryType(String s) { - _primaryType = s; - } - String get subType => _subType; - void set subType(String s) { - _subType = s; - } - String get charset => parameters["charset"]; - void set charset(String s) { - parameters["charset"] = s; - } - String _primaryType = ""; String _subType = ""; } diff --git a/sdk/lib/io/string_transformer.dart b/sdk/lib/io/string_transformer.dart index fdae9b904a7..2b02639f474 100644 --- a/sdk/lib/io/string_transformer.dart +++ b/sdk/lib/io/string_transformer.dart @@ -8,16 +8,23 @@ part of dart.io; * String encodings. */ class Encoding { - static const Encoding UTF_8 = const Encoding._internal("UTF-8"); - static const Encoding ISO_8859_1 = const Encoding._internal("ISO-8859-1"); - static const Encoding ASCII = const Encoding._internal("ASCII"); + static const Encoding UTF_8 = const Encoding._internal("utf-8"); + static const Encoding ISO_8859_1 = const Encoding._internal("iso-8859-1"); + static const Encoding ASCII = const Encoding._internal("us-ascii"); + + /** + * Name of the encoding. This will be the lower-case version of one of the + * IANA official names for the character set (see + * http://www.iana.org/assignments/character-sets/character-sets.xml) + */ + final String name; + /** * SYSTEM encoding is the current code page on Windows and UTF-8 on * Linux and Mac. */ - static const Encoding SYSTEM = const Encoding._internal("SYSTEM"); + static const Encoding SYSTEM = const Encoding._internal("system"); const Encoding._internal(String this.name); - final String name; } diff --git a/tests/standalone/io/http_advanced_test.dart b/tests/standalone/io/http_advanced_test.dart index e19365f66a8..56b51e898c2 100644 --- a/tests/standalone/io/http_advanced_test.dart +++ b/tests/standalone/io/http_advanced_test.dart @@ -145,8 +145,7 @@ class TestServer { Expect.equals("html", request.headers.contentType.subType); Expect.equals("utf-8", request.headers.contentType.parameters["charset"]); - ContentType contentType = new ContentType("text", "html"); - contentType.parameters["charset"] = "utf-8"; + ContentType contentType = new ContentType("text", "html", charset: "utf-8"); response.headers.contentType = contentType; response.close(); } @@ -347,10 +346,8 @@ Future testContentType() { httpClient.get("127.0.0.1", port, "/contenttype1") .then((request) { - ContentType contentType = new ContentType(); - contentType.value = "text/html"; - contentType.parameters["charset"] = "utf-8"; - request.headers.contentType = contentType; + request.headers.contentType = + new ContentType("text", "html", charset: "utf-8"); return request.close(); }) .then(processResponse); diff --git a/tests/standalone/io/http_headers_test.dart b/tests/standalone/io/http_headers_test.dart index 19829e79954..5d94173ff13 100644 --- a/tests/standalone/io/http_headers_test.dart +++ b/tests/standalone/io/http_headers_test.dart @@ -1,4 +1,4 @@ -// Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file +// Copyright (c) 2013, 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. @@ -205,6 +205,10 @@ void testHeaderValue() { headerValue = new HeaderValue.fromString( "xxx; aaa=bbb; ccc=\"\\\";\\a\"; ddd=\" \""); check(headerValue, "xxx", {"aaa": "bbb", "ccc": '\";a', "ddd": " "}); + headerValue = new HeaderValue("xxx", + {"aaa": "bbb", "ccc": '\";a', "ddd": " "}); + check(headerValue, "xxx", {"aaa": "bbb", "ccc": '\";a', "ddd": " "}); + headerValue = new HeaderValue.fromString( "attachment; filename=genome.jpeg;" "modification-date=\"Wed, 12 February 1997 16:29:51 -0500\""); @@ -213,6 +217,8 @@ void testHeaderValue() { "modification-date": "Wed, 12 February 1997 16:29:51 -0500" }; check(headerValue, "attachment", parameters); + headerValue = new HeaderValue("attachment", parameters); + check(headerValue, "attachment", parameters); headerValue = new HeaderValue.fromString( " attachment ;filename=genome.jpeg ;" "modification-date = \"Wed, 12 February 1997 16:29:51 -0500\"" ); @@ -238,47 +244,56 @@ void testContentType() { } ContentType contentType; - contentType = new ContentType(); + contentType = new ContentType("", ""); Expect.equals("", contentType.primaryType); Expect.equals("", contentType.subType); Expect.equals("/", contentType.value); - contentType.value = "text/html"; - Expect.equals("text", contentType.primaryType); - Expect.equals("html", contentType.subType); - Expect.equals("text/html", contentType.value); - contentType = new _ContentType.fromString("text/html"); + contentType = new ContentType.fromString("text/html"); check(contentType, "text", "html"); Expect.equals("text/html", contentType.toString()); - contentType.parameters["charset"] = "utf-8"; + contentType = new ContentType("text", "html", charset: "utf-8"); check(contentType, "text", "html", {"charset": "utf-8"}); Expect.equals("text/html; charset=utf-8", contentType.toString()); - contentType.parameters["xxx"] = "yyy"; + + contentType = new ContentType("text", + "html", + parameters: {"CHARSET": "UTF-8", "xxx": "yyy"}); check(contentType, "text", "html", {"charset": "utf-8", "xxx": "yyy"}); String s = contentType.toString(); bool expectedToString = (s == "text/html; charset=utf-8; xxx=yyy" || s == "text/html; xxx=yyy; charset=utf-8"); Expect.isTrue(expectedToString); - contentType = new _ContentType.fromString("text/html"); + contentType = new ContentType("text", + "html", + charset: "ISO-8859-1", + parameters: {"CHARSET": "UTF-8", "xxx": "yyy"}); + check(contentType, "text", "html", {"charset": "iso-8859-1", "xxx": "yyy"}); + s = contentType.toString(); + expectedToString = (s == "text/html; charset=iso-8859-1; xxx=yyy" || + s == "text/html; xxx=yyy; charset=iso-8859-1"); + Expect.isTrue(expectedToString); + + contentType = new ContentType.fromString("text/html"); check(contentType, "text", "html"); - contentType = new _ContentType.fromString(" text/html "); + contentType = new ContentType.fromString(" text/html "); check(contentType, "text", "html"); - contentType = new _ContentType.fromString("text/html; charset=utf-8"); + contentType = new ContentType.fromString("text/html; charset=utf-8"); check(contentType, "text", "html", {"charset": "utf-8"}); - contentType = new _ContentType.fromString( + contentType = new ContentType.fromString( " text/html ; charset = utf-8 "); check(contentType, "text", "html", {"charset": "utf-8"}); - contentType = new _ContentType.fromString( + contentType = new ContentType.fromString( "text/html; charset=utf-8; xxx=yyy"); check(contentType, "text", "html", {"charset": "utf-8", "xxx": "yyy"}); - contentType = new _ContentType.fromString( + contentType = new ContentType.fromString( " text/html ; charset = utf-8 ; xxx=yyy "); check(contentType, "text", "html", {"charset": "utf-8", "xxx": "yyy"}); - contentType = new _ContentType.fromString( + contentType = new ContentType.fromString( 'text/html; charset=utf-8; xxx="yyy"'); check(contentType, "text", "html", {"charset": "utf-8", "xxx": "yyy"}); - contentType = new _ContentType.fromString( + contentType = new ContentType.fromString( " text/html ; charset = utf-8 ; xxx=yyy "); check(contentType, "text", "html", {"charset": "utf-8", "xxx": "yyy"}); } @@ -294,9 +309,7 @@ void testContentTypeCache() { Expect.equals("plain", headers.contentType.subType); Expect.equals("text/plain", headers.contentType.value); headers.removeAll(HttpHeaders.CONTENT_TYPE); - Expect.equals("", headers.contentType.primaryType); - Expect.equals("", headers.contentType.subType); - Expect.equals("/", headers.contentType.value); + Expect.isNull(headers.contentType); } void testCookie() {