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: improve fallback to null on empty responses #2285

Merged
merged 7 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
4 changes: 3 additions & 1 deletion dio/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ See the [Migration Guide][] for the complete breaking changes list.**

## Unreleased

*None.*
- Graceful handling of Responses with nonzero Content-Length, Content-Type json, but empty body
- Empty responses are now transformed to `null`
Copy link
Member

Choose a reason for hiding this comment

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

Two entries? Also please write words in correct capitalization: response, content-type, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first line are the affected cases, the second line describes the result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed capitalization of "Responses".

The capitalization of the headers are like in in the official spec like https://datatracker.ietf.org/doc/html/rfc2616#section-14.13
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Length



## 5.6.0

Expand Down
24 changes: 9 additions & 15 deletions dio/lib/src/transformers/fused_transformer.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'dart:async';
import 'dart:convert';
import 'dart:typed_data';

Expand All @@ -7,6 +8,7 @@ import '../headers.dart';
import '../options.dart';
import '../transformer.dart';
import 'util/consolidate_bytes.dart';
import 'util/transform_empty_to_null.dart';

/// A [Transformer] that has a fast path for decoding UTF8-encoded JSON.
/// If the response is utf8-encoded JSON and no custom decoder is specified in the [RequestOptions], this transformer
Expand Down Expand Up @@ -118,11 +120,6 @@ class FusedTransformer extends Transformer {
// of the response or the length of the eagerly decoded response bytes
final int contentLength;

// Successful HEAD requests don't have a response body, even if the content-length header
// present.
final mightNotHaveResponseBodyDespiteContentLength =
options.method == 'HEAD';

// The eagerly decoded response bytes
// which is set if the content length is not specified and
// null otherwise (we'll feed the stream directly to the decoder in that case)
Expand All @@ -131,8 +128,7 @@ class FusedTransformer extends Transformer {
// If the content length is not specified, we need to consolidate the stream
// and count the bytes to determine if we should use an isolate
// otherwise we use the content length header
if (!hasContentLengthHeader ||
mightNotHaveResponseBodyDespiteContentLength) {
if (!hasContentLengthHeader) {
responseBytes = await consolidateBytes(responseBody.stream);
contentLength = responseBytes.length;
} else {
Expand All @@ -153,13 +149,7 @@ class FusedTransformer extends Transformer {
responseBytes ?? await consolidateBytes(responseBody.stream),
);
} else {
if (!hasContentLengthHeader || contentLength == 0) {
// This path is for backwards compatibility.
// If content-type indicates a json response,
// but the body is empty, null is returned.
// _utf8JsonDecoder.bind(responseBody.stream) would throw if the body is empty.
// So we need to check if the body is empty and return null in that case
responseBytes ??= await consolidateBytes(responseBody.stream);
if (responseBytes != null) {
if (responseBytes.isEmpty) {
return null;
}
Expand All @@ -168,7 +158,11 @@ class FusedTransformer extends Transformer {
assert(responseBytes == null);
// The content length is specified and we can feed the stream directly to the decoder,
// without eagerly decoding the response bytes first.
final decodedStream = _utf8JsonDecoder.bind(responseBody.stream);
// If the response is empty, return null;
// This is done by the DefaultNullIfEmptyStreamTransformer
final streamWithNullFallback = responseBody.stream
.transform(const DefaultNullIfEmptyStreamTransformer());
final decodedStream = _utf8JsonDecoder.bind(streamWithNullFallback);
final decoded = await decodedStream.toList();
if (decoded.isEmpty) {
return null;
Expand Down
48 changes: 48 additions & 0 deletions dio/lib/src/transformers/util/transform_empty_to_null.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import 'dart:async';
import 'dart:typed_data';

/// A [StreamTransformer] that replaces an empty stream of Uint8List with a default value
/// - the utf8-encoded string "null".
/// Feeding an empty stream to a JSON decoder will throw an exception, so this transformer
/// is used to prevent that; the JSON decoder will instead return null.
class DefaultNullIfEmptyStreamTransformer
extends StreamTransformerBase<Uint8List, Uint8List> {
const DefaultNullIfEmptyStreamTransformer();

@override
Stream<Uint8List> bind(Stream<Uint8List> stream) {
return Stream.eventTransformed(
stream,
(sink) => _DefaultIfEmptyStreamSink(sink),
);
}
}

class _DefaultIfEmptyStreamSink implements EventSink<Uint8List> {
_DefaultIfEmptyStreamSink(this._outputSink);

/// Hard-coded constant for replacement value, "null"
static final Uint8List _nullUtf8Value =
Uint8List.fromList(const [110, 117, 108, 108]);
Comment on lines +24 to +26
Copy link
Member

Choose a reason for hiding this comment

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

What does the value help with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for adding the byte sequence for "null" to the response stream, in the case that the response stream is unexpectedly empty.
So we return null instead of throwing an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that cause a side effect that user won't understand why their response becomes "null"?

Copy link
Contributor Author

@knaeckeKami knaeckeKami Aug 28, 2024

Choose a reason for hiding this comment

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

For clarification: The response won't become the String "null", it would become the value null after the json decoder decodes that byte sequence.


It has always been the behavior of Dio to return null when the ResponseType is .json and the response is empty.
Only in the recent release with the FusedTransformer there is now a corner case that if

  • the ResponseType is .json and
  • the Content-Type header is application/json and
  • the Content-Length header has a value of > 0 and
  • the response is still empty

That an exception is thrown (instead of the old behavior of returning null).
This is because we currently assume that if the Content-Length is >0, that the response body will not be empty. So the response stream is fed into the json decoder, but the decoder throws on empty streams.

This transformation is not applied for other ResponseType than .json.

I would also be okay with removing the behavior of returning null on empty responses when the ResponseType is .json but the body is empty completely, and always throw an exception in that case.
But that would be a breaking change for sure.

The behavior of the current release, where it mostly returns null on empty responses but throws an exception in certain corner cases (mostly responses that are invalid according to the HTTP spec, but still occur in the real world), is not ideal IMO.


final EventSink<Uint8List> _outputSink;
bool _hasData = false;

@override
void add(Uint8List data) {
_hasData = _hasData || data.isNotEmpty;
_outputSink.add(data);
}

@override
void addError(e, [st]) => _outputSink.addError(e, st);

@override
void close() {
if (!_hasData) {
_outputSink.add(_nullUtf8Value);
}

_outputSink.close();
}
}
43 changes: 43 additions & 0 deletions dio/test/transformer_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,30 @@ void main() {
expect(response, {'foo': 'bar'});
});

test('transformResponse transforms json array', () async {
final transformer = FusedTransformer();
const jsonString = '[{"foo": "bar"}]';
final response = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.json),
ResponseBody.fromString(
jsonString,
200,
headers: {
Headers.contentTypeHeader: ['application/json'],
Headers.contentLengthHeader: [
utf8.encode(jsonString).length.toString(),
],
},
),
);
expect(
response,
[
{'foo': 'bar'},
],
);
});

test('transforms json in background isolate', () async {
final transformer = FusedTransformer(contentLengthIsolateThreshold: 0);
final jsonString = '{"foo": "bar"}';
Expand Down Expand Up @@ -299,6 +323,25 @@ void main() {
expect(response, null);
},
);

test(
'can handle status 304 responses with content-length but empty body',
() async {
final transformer = FusedTransformer();
final response = await transformer.transformResponse(
RequestOptions(responseType: ResponseType.json),
ResponseBody(
Stream.value(Uint8List(0)),
304,
headers: {
Headers.contentTypeHeader: ['application/json'],
Headers.contentLengthHeader: ['123'],
},
),
);
expect(response, null);
},
);
});

group('consolidate bytes', () {
Expand Down