fix: improve fallback to null on empty responses by knaeckeKami · Pull Request #2285 · cfug/dio

knaeckeKami · 2024-08-18T16:27:28Z

This improves handling of the following case:

ResponseType is json
Server Content-Type is json
Server Content-Length is nonzero
Response Body is empty

This can happen in some cases according to HTTP spec (e.g. HEAD requests), but some servers return responses like this (see #2279) also in other cases, even if it violates the HTTP spec.

With #2250, in some of these cases dio would throw an exception, which caused a regression for some users; see #2279
I do believe that dio should handle these cases for gracefully,

This PR brings back the previous behavior of returning null.

New Pull Request Checklist

I have read the Documentation
I have searched for a similar pull request in the project and found none
I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
I have added the required tests to prove the fix/feature I'm adding
I have updated the documentation (if necessary)
I have run the tests without failures
I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

Co-authored-by: Jonas Uekötter <ueman@users.noreply.github.com> Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com>

…com/knaeckeKami/dio into fix/fused_transformer_null_fallback

Schwusch · 2024-08-28T08:07:11Z

This is also a problem downstream in package:retrofit, where/when content type is always JSON but response body is empty.
Is there someone that can approve this? @ueman ?

ueman · 2024-08-28T08:10:50Z

@kuhnroyal @AlexV525 Do we want to release this as 5.7.0 or as a 6.0.0? This could be considered a breaking change, but it doesn't really feel like a big change. What do you think?

knaeckeKami · 2024-08-28T08:24:01Z

I'd argue it's a bug fix, as the change occurred with #2250, and this restores the old behavior.

But of course, a bug fix can also be viewed as a breaking change if people depend on it ;)

ueman · 2024-08-28T08:25:57Z

I'd argue it's a bug fix, as the change occurred with #2250, and this restores the old behavior.

But of course, a bug fix can also be viewed as a breaking change if people depend on it ;)

Yeah, I'm tempted to release it as 5.7.0, but I still would like to hear their opinion

AlexV525

I didn't take a detailed look into what happened. In general we need to provide tests that can pass in previous specific versions and with the request changes.

AlexV525 · 2024-08-28T08:27:26Z

+- Graceful handling of Responses with nonzero Content-Length, Content-Type json, but empty body
+  - Empty responses are now transformed to `null`


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

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

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

AlexV525 · 2024-08-28T08:59:57Z

+  /// Hard-coded constant for replacement value, "null"
+  static final Uint8List _nullUtf8Value =
+      Uint8List.fromList(const [110, 117, 108, 108]);


What does the value help with?

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.

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

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.

knaeckeKami · 2024-08-28T13:28:50Z

I added a test here:
https://github.com/cfug/dio/pull/2285/files#diff-60c4759d76b4a7475d5c699664cbe409ca3cf5c6f09a403a5f3d2a4c4dd08ed6R330

This test would pass before #2250, and after this PR is merged.
With the current release, it would fail.

A user opened #2279 because of that.

The server sends an invalid response (according to the HTTP spec), but I still think that it's better to handle such cases gracefully in an HTTP client (especially if the old behavior was graceful (return null instead of throwing an exception).

Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com>

github-actions · 2024-08-28T13:44:17Z

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
dio/lib/src/transformers/fused_transformer.dart	🟢 91.3%	🟢 93.33%	🟢 2.03%
dio/lib/src/transformers/util/transform_empty_to_null.dart	🔴 0%	🟢 87.5%	🟢 87.5%
Overall Coverage	🟢 84.39%	🟢 84.46%	🟢 0.07%

Minimum allowed coverage is 0%, this run produced 84.46%

…onseDecoder (#2550) ### Problem `FusedTransformer` is the default `Transformer`. When a request sets a custom `responseDecoder` **and** the server returns an empty body with a JSON content-type (a normal `200`/`204`/`304`), the transformer's slow path runs `jsonDecode('')`, throwing `FormatException: Unexpected end of input`. `SyncTransformer` and `BackgroundTransformer` handle the identical input gracefully — they guard the empty string and return it (which dio normalizes to `null` for typed JSON responses). Only the default transformer crashes. This is the residual half of #2279: PR #2285 fixed the empty-body → null behavior for the **fast path** (no custom decoder) but never guarded the custom-decoder slow path. ### Fix Add an `isNotEmpty` check to the slow-path guard so an empty decoded body falls through to the existing `return decodedResponse;` branch, matching `SyncTransformer` / `BackgroundTransformer`: ```dart if (isJsonContent && decodedResponse != null && decodedResponse.isNotEmpty) { return jsonDecode(decodedResponse); } else if (customResponseDecoder != null) { return decodedResponse; } ``` Non-empty malformed bodies still throw, exactly as before and as the sibling transformers do (the fix is intentionally scoped to match their `isNotEmpty`-only guard). ### New Pull Request Checklist - [x] I have read the [Documentation](https://pub.dev/documentation/dio/latest/) - [x] I have searched for a similar pull request in the [project](https://github.com/cfug/dio/pulls) and found none - [x] I have updated this branch with the latest `main` branch to avoid conflicts (via merge from master or rebase) - [x] I have added the required tests to prove the fix/feature I'm adding - [x] I have updated the documentation (if necessary) — n/a, no public API change - [x] I have run the tests without failures - [x] I have updated the `CHANGELOG.md` in the corresponding package ### Additional context and info (if any) Reproduction (no network needed), calling the default transformer directly: ```dart String decoder(List<int> b, RequestOptions o, ResponseBody rb) => utf8.decode(b, allowMalformed: true); await FusedTransformer().transformResponse( RequestOptions(responseType: ResponseType.json, responseDecoder: decoder), ResponseBody.fromBytes([], 200, headers: { Headers.contentTypeHeader: [Headers.jsonContentType], }), ); // before: throws FormatException: Unexpected end of input // after: returns '' (consistent with SyncTransformer / BackgroundTransformer) ``` The added regression test (`dio/test/transformer_test.dart` → *"empty JSON body with a custom responseDecoder does not throw"*) fails without the fix and passes with it. Co-authored-by: Alex Li <github@alexv525.com>

knaeckeKami added 2 commits August 18, 2024 17:20

fix: improve fallback to null on empty responses

f73ff71

fix: improve fallback to null on empty responses

79d88f4

knaeckeKami requested a review from a team as a code owner August 18, 2024 16:27

ueman reviewed Aug 19, 2024

View reviewed changes

Comment thread dio/CHANGELOG.md Outdated

Comment thread dio/lib/src/transformers/util/transform_empty_to_null.dart Outdated

knaeckeKami and others added 4 commits August 19, 2024 08:18

Update dio/CHANGELOG.md

9c02075

Co-authored-by: Jonas Uekötter <ueman@users.noreply.github.com> Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com>

remove duplicated test after merge

61ad86e

Merge branch 'fix/fused_transformer_null_fallback' of https://github.…

eac00c9

…com/knaeckeKami/dio into fix/fused_transformer_null_fallback

replace compound bitwise assignment with regular assignment

0f9dcff

ueman approved these changes Aug 28, 2024

View reviewed changes

AlexV525 reviewed Aug 28, 2024

View reviewed changes

Update CHANGELOG.md

1591321

Signed-off-by: Martin Kamleithner <martin.kamleithner@gmail.com>

ueman merged commit d7cad71 into cfug:main Sep 3, 2024

tonytonycoder11 mentioned this pull request Jul 5, 2026

fix(dio): avoid FormatException on empty JSON body with a custom responseDecoder #2550

Merged

7 tasks

		- Graceful handling of Responses with nonzero Content-Length, Content-Type json, but empty body
		- Empty responses are now transformed to `null`

Uh oh!

Uh oh!

Conversation

knaeckeKami commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Pull Request Checklist

Additional context and info (if any)

Uh oh!

Uh oh!

Uh oh!

Schwusch commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ueman commented Aug 28, 2024

Uh oh!

knaeckeKami commented Aug 28, 2024

Uh oh!

ueman commented Aug 28, 2024

Uh oh!

AlexV525 left a comment

Choose a reason for hiding this comment

Uh oh!

AlexV525 Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

ueman Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

AlexV525 Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

AlexV525 Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

knaeckeKami Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knaeckeKami commented Aug 28, 2024

Uh oh!

github-actions Bot commented Aug 28, 2024

Code Coverage Report: Only Changed Files listed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knaeckeKami commented Aug 18, 2024 •

edited

Loading

Schwusch commented Aug 28, 2024 •

edited

Loading

knaeckeKami Aug 28, 2024 •

edited

Loading