Skip to content
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

Fix Default Encoding for application/json Content-Type in HTTP Responses #1422

Merged
merged 11 commits into from
Feb 7, 2025
2 changes: 1 addition & 1 deletion pkgs/http/lib/src/multipart_file.dart
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class MultipartFile {
factory MultipartFile.fromString(String field, String value,
{String? filename, MediaType? contentType}) {
contentType ??= MediaType('text', 'plain');
var encoding = encodingForCharset(contentType.parameters['charset'], utf8);
var encoding = encodingForContentTypeHeader(contentType, utf8);
contentType = contentType.change(parameters: {'charset': encoding.name});

return MultipartFile.fromBytes(field, encoding.encode(value),
Expand Down
14 changes: 8 additions & 6 deletions pkgs/http/lib/src/response.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ class Response extends BaseResponse {
///
/// This is converted from [bodyBytes] using the `charset` parameter of the
/// `Content-Type` header field, if available. If it's unavailable or if the
/// encoding name is unknown, [latin1] is used by default, as per
/// [RFC 2616][].
/// encoding name is unknown, [utf8] is used by default,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is only when content-type is UTF-8 right? So could you clarify that in your comments e.g.

If it's unavailable or if the encoding name is unknown:

  1. [utf8] is used when the content-type is 'application/json' (see [RFC 3629]).
  2. [latin1] is used in all other cases (see [RFC 2616][]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brianquinlan, thanks for the review.
Let me know if anything else needs improvement! 🚀

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this is great! Thank you so much for your contribution!

/// as per [RFC3629][].
///
/// [RFC 2616]: http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html
/// [RFC3629]:https://www.rfc-editor.org/rfc/rfc3629.
String get body => _encodingForHeaders(headers).decode(bodyBytes);

/// Creates a new HTTP response with a string body.
Expand Down Expand Up @@ -66,10 +66,12 @@ class Response extends BaseResponse {

/// Returns the encoding to use for a response with the given headers.
///
/// Defaults to [latin1] if the headers don't specify a charset or if that
/// charset is unknown.
/// If the `Content-Type` header specifies a charset, it will use that charset.
/// If no charset is provided or the charset is unknown:
/// - Defaults to [utf8] if the `Content-Type` is `application/json` (since JSON is defined to use UTF-8 by default).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrap to 80 columns

/// - Otherwise, defaults to [latin1] for compatibility.
Encoding _encodingForHeaders(Map<String, String> headers) =>
encodingForCharset(_contentTypeForHeaders(headers).parameters['charset']);
encodingForContentTypeHeader(_contentTypeForHeaders(headers));

/// Returns the [MediaType] object for the given headers' content-type.
///
Expand Down
28 changes: 22 additions & 6 deletions pkgs/http/lib/src/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import 'dart:async';
import 'dart:convert';
import 'dart:typed_data';

import 'package:http_parser/http_parser.dart';

import 'byte_stream.dart';

/// Converts a [Map] from parameter names to values to a URL query string.
Expand All @@ -18,13 +20,27 @@ String mapToQuery(Map<String, String> map, {required Encoding encoding}) =>
'=${Uri.encodeQueryComponent(e.value, encoding: encoding)}')
.join('&');

/// Returns the [Encoding] that corresponds to [charset].
/// Determines the appropriate [Encoding] based on the given [contentTypeHeader]
///
/// Returns [fallback] if [charset] is null or if no [Encoding] was found that
/// corresponds to [charset].
Encoding encodingForCharset(String? charset, [Encoding fallback = latin1]) {
if (charset == null) return fallback;
return Encoding.getByName(charset) ?? fallback;
/// - If the `Content-Type` is `application/json` and no charset is specified,
/// it defaults to [utf8].
/// - If a charset is specified in the parameters,
/// it attempts to find a matching [Encoding].
/// - If no charset is specified or the charset is unknown,
/// it falls back to the provided [fallback], which defaults to [latin1].
Encoding encodingForContentTypeHeader(MediaType contentTypeHeader,
[Encoding fallback = latin1]) {
final charset = contentTypeHeader.parameters['charset'];

// Default to utf8 for application/json when charset is unspecified.
if (contentTypeHeader.type == 'application' &&
contentTypeHeader.subtype == 'json' &&
charset == null) {
return utf8;
}

// Attempt to find the encoding or fall back to the default.
return charset != null ? Encoding.getByName(charset) ?? fallback : fallback;
}

/// Returns the [Encoding] that corresponds to [charset].
Expand Down
18 changes: 18 additions & 0 deletions pkgs/http/test/response_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.

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

import 'package:http/http.dart' as http;
import 'package:test/test.dart';
Expand Down Expand Up @@ -45,6 +46,23 @@ void main() {
headers: {'content-type': 'text/plain; charset=iso-8859-1'});
expect(response.body, equals('föøbãr'));
});
test('test decoding with empty charset if content type is application/json',
() {
final utf8Bytes = utf8.encode('{"foo":"Привет, мир!"}');
var response = http.Response.bytes(utf8Bytes, 200,
headers: {'content-type': 'application/json'});
expect(response.body, equals('{"foo":"Привет, мир!"}'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, there should be one except per test because you are testing a single scenario.

Could you add tests for these additional scenarios:

  1. test .body with a non-empty charset with content-type application/json data (e.g. with Russian and iso-8859-1)
  2. test .body with an empty charset with a content-type of test/plain (e.g. with Russian to verify that the default of iso-8859-1 is used)


final chineseUtf8Bytes = utf8.encode('{"foo":"你好,世界!"}');
var responseChinese = http.Response.bytes(chineseUtf8Bytes, 200,
headers: {'content-type': 'application/json'});
expect(responseChinese.body, equals('{"foo":"你好,世界!"}'));

final koreanUtf8Bytes = utf8.encode('{"foo":"안녕하세요, 세계!"}');
var responseKorean = http.Response.bytes(koreanUtf8Bytes, 200,
headers: {'content-type': 'application/json'});
expect(responseKorean.body, equals('{"foo":"안녕하세요, 세계!"}'));
});
});

group('.fromStream()', () {
Expand Down
Loading