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.

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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 29 additions & 29 deletions lib/src/body.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import 'dart:convert';
import 'package:async/async.dart';
import 'package:collection/collection.dart';

import 'empty_body.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 All @@ -27,6 +30,9 @@ class Body {
/// determined efficiently.
final int contentLength;

/// An empty stream for use with empty bodies.
static const _emptyStream = const Stream.empty();
Copy link
Member

Choose a reason for hiding this comment

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

I meant store an instance of Body statically:

static _empty = new Body._(const Stream.empty(), null, 0);

Creating a static field for a const value doesn't do anything since const constructors are already canonicalized.

Copy link
Author

Choose a reason for hiding this comment

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

Was looking at implementing this but the issue is that calling read removes the stream.

Do you instead want an EmptyBody class that lets you call read() multiple times?

Copy link
Member

Choose a reason for hiding this comment

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

Do you instead want an EmptyBody class that lets you call read() multiple times?

Yeah, I think this is the way to go. That matches the behavior of const Stream.empty(), too.


Body._(this._stream, this.encoding, this.contentLength);

/// Converts [body] to a byte stream and wraps it in a [Body].
Expand All @@ -36,25 +42,23 @@ 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 const EmptyBody();
}

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]);
}

encoding ??= UTF8;

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

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 +72,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 All @@ -94,3 +85,12 @@ class Body {
return stream;
}
}

class _EmptyBody implements Body {
const _EmptyBody();

Encoding get encoding => UTF8;
int get contentLength => 0;

Stream<List<int>> read() => const Stream<List<int>>.empty();
}
25 changes: 25 additions & 0 deletions lib/src/empty_body.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) 2017, 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.

import 'dart:async';
import 'dart:convert';

import 'body.dart';

/// An empty request body.
///
/// Used as an optimization when a `null` body is used.
class EmptyBody implements Body {
/// An empty body is not encoded.
Encoding get encoding => null;

/// An empty body has no length.
int get contentLength => 0;

/// Creates an instance of [EmptyBody].
const EmptyBody();

/// Returns an empty [Stream] representing the body.
Stream<List<int>> read() => const Stream<List<int>>.empty();
}
166 changes: 122 additions & 44 deletions lib/src/message.dart
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 @@ -61,12 +60,27 @@ abstract class Message {
{Encoding encoding,
Map<String, String> headers,
Map<String, Object> context})
: this._(new Body(body, encoding), headers, context);
: this._forwardToContentType(body, encoding,
_determineMediaType(body, encoding, headers), headers, context);

Message._(Body body, Map<String, String> headers, Map<String, Object> context)
Message.withContentType(Body body, MediaType contentType,
Copy link
Member

Choose a reason for hiding this comment

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

I don't want this constructor here. The shape of the constructors from before this CL is generally correct; the only change should be adding a {bool bodyIsMap: false} parameter to new Message._() and _adjustHeaders().

Copy link
Author

Choose a reason for hiding this comment

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

I had planned to use it with http.MultiPart since the MediaType requires a boundary which would be randomly generated.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have _adjustHeaders() do some logic like

if (body is MultipartBody) {
  contentType = "multipart/mixed; boundary=${body.boundary}";
}

Map<String, String> headers, Map<String, Object> context)
: this._fromValues(
body, _adjustHeaders(headers, body, contentType), context);

Message._forwardToContentType(body, Encoding encoding, MediaType contentType,
Map<String, String> headers, Map<String, Object> context)
: this.withContentType(
new Body(body, encoding), contentType, headers, context);

/// Creates a new [Message].
///
/// This constructor should be used when no computation is required for the
/// [body], [headers] or [context].
Message._fromValues(
Body body, Map<String, String> headers, Map<String, Object> context)
: _body = body,
headers = new HttpUnmodifiableMap<String>(_adjustHeaders(headers, body),
ignoreKeyCase: true),
headers = new HttpUnmodifiableMap<String>(headers, ignoreKeyCase: true),
context =
new HttpUnmodifiableMap<Object>(context, ignoreKeyCase: false);

Expand Down Expand Up @@ -144,55 +158,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;

/// Returns whether [headers] declares the same encoding as [body].
bool _sameEncoding(Map<String, String> headers, Body body) {
if (body.encoding == null) return true;
if (bodyContentLength >= 0) {
var bodyContentHeader = bodyContentLength.toString();

var contentType = getHeader(headers, 'content-type');
if (contentType == null) return false;
if (getHeader(headers, 'content-length') != bodyContentHeader) {
return bodyContentHeader;
}
}

return '';
}

var charset = new MediaType.parse(contentType).parameters['charset'];
return Encoding.getByName(charset) == body.encoding;
/// 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();

// Neither are set so don't modify it
if ((headerContentType == null) && (bodyContentType == null)) {
return '';
}

// The value of bodyContentType will have the overridden values so use that
return headerContentType != bodyContentType ? bodyContentType : '';
}
}