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

🐛 Only produce null response body when the response is a JSON type #1874

Merged
merged 8 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
2 changes: 1 addition & 1 deletion dio/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ See the [Migration Guide][] for the complete breaking changes list.**

## Unreleased

*None.*
- Only produce null response body when `ResponseType.json`.

## 5.2.1+1

Expand Down
17 changes: 14 additions & 3 deletions dio/lib/src/dio_mixin.dart
Original file line number Diff line number Diff line change
Expand Up @@ -537,10 +537,9 @@ abstract class DioMixin implements Dio {
// Initiate Http requests
Future<Response<dynamic>> _dispatchRequest<T>(RequestOptions reqOpt) async {
final cancelToken = reqOpt.cancelToken;
ResponseBody responseBody;
try {
final stream = await _transformData(reqOpt);
responseBody = await httpClientAdapter.fetch(
final responseBody = await httpClientAdapter.fetch(
reqOpt,
stream,
cancelToken?.whenCancel,
Expand All @@ -559,7 +558,19 @@ abstract class DioMixin implements Dio {
);
final statusOk = reqOpt.validateStatus(responseBody.statusCode);
if (statusOk || reqOpt.receiveDataWhenStatusError == true) {
ret.data = await transformer.transformResponse(reqOpt, responseBody);
Object? data = await transformer.transformResponse(
reqOpt,
responseBody,
);
// Make the response as null before returned as JSON.
if (data is String &&
AlexV525 marked this conversation as resolved.
Show resolved Hide resolved
data.isEmpty &&
T != dynamic &&
T != String &&
reqOpt.responseType == ResponseType.json) {
data = null;
}
ret.data = data;
} else {
await responseBody.stream.listen(null).cancel();
}
Expand Down
5 changes: 3 additions & 2 deletions dio/lib/src/transformer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ abstract class Transformer {
/// [transformResponse] allows changes to the response data before
/// it is passed to [ResponseInterceptor].
///
/// **Note**: As an agreement, you must return the [response]
/// **Note**: As an agreement, you must return the [responseBody]
/// when the Options.responseType is [ResponseType.stream].
Future transformResponse(RequestOptions options, ResponseBody response);
// TODO(AlexV525): Add generic type for the method in v6.0.0.
Future transformResponse(RequestOptions options, ResponseBody responseBody);

/// Deep encode the [Map<String, dynamic>] to percent-encoding.
/// It is mostly used with the "application/x-www-form-urlencoded" content-type.
Expand Down
79 changes: 45 additions & 34 deletions dio/lib/src/transformers/sync_transformer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,86 +42,97 @@ class SyncTransformer extends Transformer {
}
}

/// As an agreement, we return the [response] when the
/// Options.responseType is [ResponseType.stream].
@override
Future<dynamic> transformResponse(
RequestOptions options,
ResponseBody response,
ResponseBody responseBody,
) async {
final responseType = options.responseType;
// Do not handled the body for streams.
if (options.responseType == ResponseType.stream) {
return response;
return responseBody;
}
int length = 0;
int received = 0;

final showDownloadProgress = options.onReceiveProgress != null;
final int totalLength;
if (showDownloadProgress) {
length = int.parse(
response.headers[Headers.contentLengthHeader]?.first ?? '-1',
totalLength = int.parse(
responseBody.headers[Headers.contentLengthHeader]?.first ?? '-1',
);
} else {
totalLength = 0;
}
final completer = Completer();
final stream = response.stream.transform<Uint8List>(

int received = 0;
final stream = responseBody.stream.transform<Uint8List>(
StreamTransformer.fromHandlers(
handleData: (data, sink) {
sink.add(data);
if (showDownloadProgress) {
received += data.length;
options.onReceiveProgress?.call(received, length);
options.onReceiveProgress?.call(received, totalLength);
}
},
),
);

final streamCompleter = Completer<void>();
int finalLength = 0;
// Keep references to the data chunks and concatenate them later.
final chunks = <Uint8List>[];
int finalSize = 0;
final StreamSubscription subscription = stream.listen(
final subscription = stream.listen(
(chunk) {
finalSize += chunk.length;
finalLength += chunk.length;
chunks.add(chunk);
},
onError: (Object error, StackTrace stackTrace) {
completer.completeError(error, stackTrace);
streamCompleter.completeError(error, stackTrace);
},
onDone: () {
streamCompleter.complete();
},
onDone: () => completer.complete(),
cancelOnError: true,
);
options.cancelToken?.whenCancel.then((_) {
return subscription.cancel();
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure about how this line will behave (whether we should complete the completer or not). It's a void future and we await for the completer in the outside scope.

});
await completer.future;
// Copy all chunks into a final Uint8List.
final responseBytes = Uint8List(finalSize);
await streamCompleter.future;

// Copy all chunks into the final bytes.
final responseBytes = Uint8List(finalLength);
int chunkOffset = 0;
for (final chunk in chunks) {
responseBytes.setAll(chunkOffset, chunk);
chunkOffset += chunk.length;
}

if (options.responseType == ResponseType.bytes) {
// Return the finalized bytes if the response type is bytes.
if (responseType == ResponseType.bytes) {
return responseBytes;
}

final String? responseBody;
final isJsonContent = Transformer.isJsonMimeType(
responseBody.headers[Headers.contentTypeHeader]?.first,
);
final String? response;
if (options.responseDecoder != null) {
responseBody = options.responseDecoder!(
response = options.responseDecoder!(
responseBytes,
options,
response..stream = Stream.empty(),
responseBody..stream = Stream.empty(),
);
} else if (responseBytes.isNotEmpty) {
responseBody = utf8.decode(responseBytes, allowMalformed: true);
} else if (!isJsonContent || responseBytes.isNotEmpty) {
response = utf8.decode(responseBytes, allowMalformed: true);
} else {
responseBody = null;
response = null;
}
if (responseBody != null &&
responseBody.isNotEmpty &&
options.responseType == ResponseType.json &&
Transformer.isJsonMimeType(
response.headers[Headers.contentTypeHeader]?.first,
)) {
return jsonDecodeCallback(responseBody);

if (response != null &&
response.isNotEmpty &&
responseType == ResponseType.json &&
isJsonContent) {
return jsonDecodeCallback(response);
}
return responseBody;
return response;
}
}
4 changes: 2 additions & 2 deletions dio/test/mock/http_mock.mocks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -672,8 +672,8 @@ class MockTransformer extends _i1.Mock implements _i5.Transformer {
returnValue: Future<String>.value('')) as _i4.Future<String>);
@override
_i4.Future<dynamic> transformResponse(
_i6.RequestOptions? options, _i7.ResponseBody? response) =>
_i6.RequestOptions? options, _i7.ResponseBody? responseBody) =>
(super.noSuchMethod(
Invocation.method(#transformResponse, [options, response]),
Invocation.method(#transformResponse, [options, responseBody]),
returnValue: Future<dynamic>.value()) as _i4.Future<dynamic>);
}
8 changes: 8 additions & 0 deletions dio/test/options_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,14 @@ void main() {
r10.requestOptions.contentType,
startsWith(Headers.multipartFormDataContentType),
);

// Regression: https://github.com/cfug/dio/issues/1834
final r11 = await dio.get('');
expect(r11.data, '');
final r12 = await dio.get<Map>('');
expect(r12.data, null);
final r13 = await dio.get<Map<String, Object>>('');
expect(r13.data, null);
});

test('default content-type 2', () async {
Expand Down
52 changes: 52 additions & 0 deletions dio/test/transformer_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import 'package:dio/dio.dart';
import 'package:test/test.dart';

void main() {
group(BackgroundTransformer(), () {
test('transformResponse transforms the request', () async {
final transformer = BackgroundTransformer();
final response = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.json),
ResponseBody.fromString(
'{"foo": "bar"}',
200,
headers: {
Headers.contentTypeHeader: ['application/json'],
},
),
);
expect(response, {'foo': 'bar'});
});
});

// Regression: https://github.com/cfug/dio/issues/1834
test('null response body only when the response is JSON', () async {
final transformer = BackgroundTransformer();
final r1 = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.json),
ResponseBody.fromBytes([], 200),
);
expect(r1, '');
final r2 = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.bytes),
ResponseBody.fromBytes([], 200),
);
expect(r2, []);
final r3 = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.plain),
ResponseBody.fromBytes([], 200),
);
expect(r3, '');
final r4 = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.json),
ResponseBody.fromBytes(
[],
200,
headers: {
Headers.contentTypeHeader: [Headers.jsonContentType],
},
),
);
expect(r4, null);
});
}
21 changes: 0 additions & 21 deletions dio/test/transformers/background_transformer_test.dart

This file was deleted.

4 changes: 2 additions & 2 deletions example/lib/transformer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ class MyTransformer extends BackgroundTransformer {
@override
Future transformResponse(
RequestOptions options,
ResponseBody response,
ResponseBody responseBody,
) async {
options.extra['self'] = 'XX';
return super.transformResponse(options, response);
return super.transformResponse(options, responseBody);
}
}

Expand Down