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

default emptyResponseAllows returns false unless HTTP Method is .head #2770

Closed
jvannoord opened this Issue Mar 27, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@jvannoord
Copy link
Contributor

commented Mar 27, 2019

What did you do?

I'm deserializing a response from an HTTP PUT Method and returns an empty status code (204).

What did you expect to happen?

My instinct was that ResponseSerializer.emptyResponseAllowed(forRequest:response:) would return true if either the http method was HEAD or the status code was 204/205.

If I were to have written this method, it would have looked like this:

requestAllowsEmptyResponseData(request) == true || responseAllowsEmptyResponseData(response) == true

What happened instead?

It appears that in the default case, the code returns true only if the http method is HEAD AND the status code to be 204/205. Below is the current code.

requestAllowsEmptyResponseData(request) ?? responseAllowsEmptyResponseData(response) ?? false

Again, I'm merely asking if this is logical AND is intentional or not.

The workaround is easy: just need to explicitly enumerate all of the HTTP methods that can return empty responses.

Alamofire Environment

Alamofire version: 5.0.0-beta3
Xcode version: 10.1 / 10.2
Swift version: 4.2
Platform(s) running Alamofire: iOS

@jshier

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

My first thought is that this is a bug from being too clever during cleanup of these methods. It should certainly be what you've put. Seems like we'll need a fix and some testing around this.

@jshier

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@jvannoord Give the bug/2770-empty-response-logic branch a shot, that should fix it. We'll roll this into the next beta, I just need to write some tests for it. Thanks for the report!

@jvannoord

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Whoa, just like that! Thanks! I was gonna volunteer, but you. are. on. it.

@ruslankaminsky

This comment has been minimized.

Copy link

commented Mar 28, 2019

I've just encountered this issue today morning, after updating from v4.8 to v5 beta 3. And noticed that server responses with code 204 and empty body started to be handled as errors with "Alamofire.AFError.ResponseSerializationFailureReason.inputDataNilOrZeroLength" error.
I'm happy that it will be fixed soon.

cnoon added a commit that referenced this issue Mar 29, 2019

Fixed up emptyResponseAllowed logic (#2770 and #2772)
* Fix emptyResponseAllowed logic.

* Update empty response logic and test.

* Whitespace cleanup.

* Add doc comment for URLRequest extension.

* Make all Results into AFResults for consistency.
@cnoon

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

This issue should be resolved in the 5.0.0-beta.4 release. Please test it out and let us know!

Cheers. 🍻

@cnoon cnoon closed this Mar 29, 2019

@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 29, 2019

cysp added a commit to ScentreGroup/Alamofire that referenced this issue Apr 11, 2019

Fixed up emptyResponseAllowed logic (Alamofire#2770 and Alamofire#2772)
* Fix emptyResponseAllowed logic.

* Update empty response logic and test.

* Whitespace cleanup.

* Add doc comment for URLRequest extension.

* Make all Results into AFResults for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.