Skip to content

Fix content type within Message #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Fix content type within Message #120

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
75be84a
Fix content type
donny-dont Sep 22, 2017
928d7a1
Add fromValues constructor
donny-dont Sep 27, 2017
5c7b451
Return an emptyStream with null body
donny-dont Sep 28, 2017
7da22ce
Review comments
donny-dont Sep 28, 2017
d05187a
unused
donny-dont Sep 28, 2017
9f5fbf4
Add EmptyBody implementation
donny-dont Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 13 additions & 29 deletions lib/src/body.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import 'dart:convert';
import 'package:async/async.dart';
import 'package:collection/collection.dart';

import 'utils.dart';

/// The body of a request or response.
///
/// This tracks whether the body has been read. It's separate from [Message]
Expand Down Expand Up @@ -36,25 +38,20 @@ class Body {
/// used to convert it to a [Stream<List<int>>].
factory Body(body, [Encoding encoding]) {
if (body is Body) return body;
if (body == null)
return new Body._(new Stream.fromIterable([]), encoding, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Stream.fromIterable() could be const Stream.empty(). It might be even cleaner to return a static _empty field here, though, to avoid any allocations in the common case of an empty body.

Nit: use curly braces.


Stream<List<int>> stream;
int contentLength;
if (body == null) {
contentLength = 0;
stream = new Stream.fromIterable([]);
} else if (body is String) {
if (encoding == null) {
var encoded = UTF8.encode(body);
// If the text is plain ASCII, don't modify the encoding. This means
// that an encoding of "text/plain" will stay put.
if (!_isPlainAscii(encoded, body.length)) encoding = UTF8;
contentLength = encoded.length;
stream = new Stream.fromIterable([encoded]);
} else {
var encoded = encoding.encode(body);
contentLength = encoded.length;
stream = new Stream.fromIterable([encoded]);
}

if (body is Map) {
body = mapToQuery(body, encoding: encoding);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this in the if/else chain with the other type tests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of body becomes a String at this point so it will end up in the is String which would then encode it.


if (body is String) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to the branch where encoding == null?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should never be null since the encoding is calculated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean when it's set to UTF8 above? But then you're marking ASCII-only bodies as UTF8 when a text/plain content type would do. The old behavior here was correct, there's no need to change it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll take a look at what 0.11 was doing here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 0.11, just the previous code in master.

var encoded = encoding.encode(body);
contentLength = encoded.length;
stream = new Stream.fromIterable([encoded]);
} else if (body is List) {
contentLength = body.length;
stream = new Stream.fromIterable([DelegatingList.typed(body)]);
Expand All @@ -68,19 +65,6 @@ class Body {
return new Body._(stream, encoding, contentLength);
}

/// Returns whether [bytes] is plain ASCII.
///
/// [codeUnits] is the number of code units in the original string.
static bool _isPlainAscii(List<int> bytes, int codeUnits) {
// Most non-ASCII code units will produce multiple bytes and make the text
// longer.
if (bytes.length != codeUnits) return false;

// Non-ASCII code units between U+0080 and U+009F produce 8-bit characters
// with the high bit set.
return bytes.every((byte) => byte & 0x80 == 0);
}

/// Returns a [Stream] representing the body.
///
/// Can only be called once.
Expand Down
188 changes: 122 additions & 66 deletions lib/src/message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import 'dart:async';
import 'dart:convert';

import 'package:collection/collection.dart';
import 'package:http_parser/http_parser.dart';

import 'body.dart';
Expand Down Expand Up @@ -45,6 +44,9 @@ abstract class Message {
/// This can be read via [read] or [readAsString].
final Body _body;

/// The parsed version of the Content-Type header in [headers].
final MediaType _contentType;

/// Creates a new [Message].
///
/// [body] is the message body. It may be either a [String], a [List<int>], a
Expand All @@ -61,14 +63,23 @@ abstract class Message {
{Encoding encoding,
Map<String, String> headers,
Map<String, Object> context})
: this._(new Body(body, encoding), headers, context);
: this.__(body, _determineMediaType(body, encoding, headers), headers,
context);

Message.__(body, MediaType contentType, Map<String, String> headers,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__ isn't a good name for a constructor--it doesn't communicate what the difference is between this and the other unnamed constructor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K I think I need to rename this anyways so it could be used in the multipart implementation.

I also didn't like fromValues but didn't have any better ideas last night.

Map<String, Object> context)
: this._(new Body(body, encodingForMediaType(contentType, null)),
contentType, headers, context);

Message._(Body body, Map<String, String> headers, Map<String, Object> context)
Message._(Body body, MediaType contentType, Map<String, String> headers,
Map<String, Object> context)
: _body = body,
headers = new HttpUnmodifiableMap<String>(_adjustHeaders(headers, body),
headers = new HttpUnmodifiableMap<String>(
_adjustHeaders(headers, body, contentType),
ignoreKeyCase: true),
context =
new HttpUnmodifiableMap<Object>(context, ignoreKeyCase: false);
new HttpUnmodifiableMap<Object>(context, ignoreKeyCase: false),
_contentType = contentType;

/// If `true`, the stream returned by [read] won't emit any bytes.
///
Expand All @@ -84,6 +95,7 @@ abstract class Message {
_contentLengthCache = int.parse(headers['content-length']);
return _contentLengthCache;
}

int _contentLengthCache;

/// The MIME type declared in [headers].
Expand All @@ -92,11 +104,7 @@ abstract class Message {
/// the MIME type, without any Content-Type parameters.
///
/// If [headers] doesn't have a Content-Type header, this will be `null`.
String get mimeType {
var contentType = _contentType;
if (contentType == null) return null;
return contentType.mimeType;
}
String get mimeType => _contentType?.mimeType;

/// The encoding of the body returned by [read].
///
Expand All @@ -105,23 +113,7 @@ abstract class Message {
///
/// If [headers] doesn't have a Content-Type header or it specifies an
/// encoding that [dart:convert] doesn't support, this will be `null`.
Encoding get encoding {
var contentType = _contentType;
if (contentType == null) return null;
if (!contentType.parameters.containsKey('charset')) return null;
return Encoding.getByName(contentType.parameters['charset']);
}

/// The parsed version of the Content-Type header in [headers].
///
/// This is cached for efficient access.
MediaType get _contentType {
if (_contentTypeCache != null) return _contentTypeCache;
if (!headers.containsKey('content-type')) return null;
_contentTypeCache = new MediaType.parse(headers['content-type']);
return _contentTypeCache;
}
MediaType _contentTypeCache;
Encoding get encoding => _body.encoding;

/// Returns the message body as byte chunks.
///
Expand All @@ -144,55 +136,119 @@ abstract class Message {
/// changes.
Message change(
{Map<String, String> headers, Map<String, Object> context, body});
}

/// Adds information about encoding to [headers].
///
/// Returns a new map without modifying [headers].
Map<String, String> _adjustHeaders(Map<String, String> headers, Body body) {
var sameEncoding = _sameEncoding(headers, body);
if (sameEncoding) {
if (body.contentLength == null ||
getHeader(headers, 'content-length') == body.contentLength.toString()) {
return headers ?? const HttpUnmodifiableMap.empty();
} else if (body.contentLength == 0 &&
(headers == null || headers.isEmpty)) {
return const HttpUnmodifiableMap.empty();
/// Determines the media type based on the [headers], [encoding] and [body].
static MediaType _determineMediaType(
body, Encoding encoding, Map<String, String> headers) =>
_headerMediaType(headers, encoding) ?? _defaultMediaType(body, encoding);

static MediaType _defaultMediaType(body, Encoding encoding) {
//if (body == null) return null;

var parameters = {'charset': encoding?.name ?? UTF8.name};

if (body is String) {
return new MediaType('text', 'plain', parameters);
} else if (body is Map) {
return new MediaType('application', 'x-www-form-urlencoded', parameters);
} else if (encoding != null) {
return new MediaType('application', 'octet-stream', parameters);
}

return null;
}

var newHeaders = headers == null
? new CaseInsensitiveMap<String>()
: new CaseInsensitiveMap<String>.from(headers);

if (!sameEncoding) {
if (newHeaders['content-type'] == null) {
newHeaders['content-type'] =
'application/octet-stream; charset=${body.encoding.name}';
} else {
var contentType = new MediaType.parse(newHeaders['content-type'])
.change(parameters: {'charset': body.encoding.name});
newHeaders['content-type'] = contentType.toString();
}
static MediaType _headerMediaType(
Map<String, String> headers, Encoding encoding) {
var contentTypeHeader = getHeader(headers, 'content-type');
if (contentTypeHeader == null) return null;

var contentType = new MediaType.parse(contentTypeHeader);
var parameters = {
'charset':
encoding?.name ?? contentType.parameters['charset'] ?? UTF8.name
};

return contentType.change(parameters: parameters);
}

if (body.contentLength != null) {
var coding = newHeaders['transfer-encoding'];
if (coding == null || equalsIgnoreAsciiCase(coding, 'identity')) {
newHeaders['content-length'] = body.contentLength.toString();
/// Adjusts the [headers] to include information from the [body].
///
/// Returns a new map without modifying [headers].
///
/// The following headers could be added or modified.
/// * content-length
/// * content-type
static Map<String, String> _adjustHeaders(
Map<String, String> headers, Body body, MediaType contentType) {
var modified = <String, String>{};

var contentLengthHeader = _adjustContentLengthHeader(headers, body);
if (contentLengthHeader.isNotEmpty) {
modified['content-length'] = contentLengthHeader;
}

var contentTypeHeader = _adjustContentTypeHeader(headers, contentType);
if (contentTypeHeader.isNotEmpty) {
modified['content-type'] = contentTypeHeader;
}

if (modified.isEmpty) {
return headers ?? const HttpUnmodifiableMap.empty();
}

var newHeaders = headers == null
? new CaseInsensitiveMap<String>()
: new CaseInsensitiveMap<String>.from(headers);

newHeaders.addAll(modified);

return newHeaders;
}

return newHeaders;
}
/// Checks the `content-length` header to see if it requires modification.
///
/// Returns an empty string when no modification is required, otherwise it
/// returns the value to set.
///
/// If there is a contentLength specified within the [body] and it does not
/// match what is specified in the [headers] it will be modified to the body's
/// value.
static String _adjustContentLengthHeader(
Map<String, String> headers, Body body) {
var bodyContentLength = body.contentLength ?? -1;

if (bodyContentLength >= 0) {
var bodyContentHeader = bodyContentLength.toString();

if (getHeader(headers, 'content-length') != bodyContentHeader) {
return bodyContentHeader;
}
}

return '';
}

/// Returns whether [headers] declares the same encoding as [body].
bool _sameEncoding(Map<String, String> headers, Body body) {
if (body.encoding == null) return true;
/// Checks the `content-type` header to see if it requires modification.
///
/// Returns an empty string when no modification is required, otherwise it
/// returns the value to set.
///
/// If the contentType within [body] is different than the one specified in the
/// [headers] then body's value will be used. The [headers] were already used
/// when creating the body's contentType so this will only actually change
/// things when headers did not contain a `content-type`.
static String _adjustContentTypeHeader(
Map<String, String> headers, MediaType contentType) {
var headerContentType = getHeader(headers, 'content-type');
var bodyContentType = contentType?.toString();

var contentType = getHeader(headers, 'content-type');
if (contentType == null) return false;
// Neither are set so don't modify it
if ((headerContentType == null) && (bodyContentType == null)) {
return '';
}

var charset = new MediaType.parse(contentType).parameters['charset'];
return Encoding.getByName(charset) == body.encoding;
// The value of bodyContentType will have the overridden values so use that
return headerContentType != bodyContentType ? bodyContentType : '';
}
}
6 changes: 6 additions & 0 deletions lib/src/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'dart:convert';
import 'dart:typed_data';

import 'package:collection/collection.dart';
import 'package:http_parser/http_parser.dart';

import 'http_unmodifiable_map.dart';

Expand Down Expand Up @@ -53,6 +54,11 @@ List<String> split1(String toSplit, String pattern) {
];
}

Encoding encodingForMediaType(MediaType type, [Encoding fallback = LATIN1]) {
if (type == null) return null;
return encodingForCharset(type.parameters['charset'], fallback);
}

/// Returns the [Encoding] that corresponds to [charset]. Returns [fallback] if
/// [charset] is null or if no [Encoding] was found that corresponds to
/// [charset].
Expand Down