Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

send(_:) method always tries to decode empty response since newest version #57

Closed
Pomanks opened this issue Sep 12, 2022 · 6 comments
Closed
Labels

Comments

@Pomanks
Copy link
Contributor

Pomanks commented Sep 12, 2022

Hi 👋🏻

When declaring a method with an optional response body (see example), the call only results in a decoding error because the method tries to decode an empty response.

Optional response body method:

func getWatching(slug: String, extended info: ExtendedInfo?) async throws -> Response<Users.GetWatching.Response?>

Error:

dataCorrupted(Swift.DecodingError.Context(
    codingPath: [], 
    debugDescription: "The given data was not valid JSON.", 
    underlyingError: Optional(Error Domain=NSCocoaErrorDomain Code=3840 "Unable to parse empty data." UserInfo={NSDebugDescription=Unable to parse empty data.})
    )
)

The same code worked in previous version though (it simply returned nil).

@Pomanks Pomanks changed the title Optional response body not decoded anymore since newest version send(_:) method always tries to decode empty response since newest version Sep 12, 2022
@kean
Copy link
Owner

kean commented Sep 12, 2022

The optional variant was removed in v1.0 to avoid "polluting" the API – this feature is rarely needed. I think it should be possible to implement it without introducing new public methods. It's something worth exploring. PRs are welcome.

@kean kean added the feature label Sep 12, 2022
@Pomanks
Copy link
Contributor Author

Pomanks commented Sep 13, 2022

A potential solution would be to treat the empty response as an error, even if the status code is not…
The implementation is not public and only located in the delegate:

    func client(_ client: APIClient, validateResponse response: HTTPURLResponse, data: Data, task: URLSessionTask) throws {
        guard (200..<300).contains(response.statusCode) else {
            throw APIError.unacceptableStatusCode(response.statusCode)
        }
        if response.statusCode == 204, data.isEmpty {
            throw APIError.emptyResponse(response.statusCode)
        }
    }

It would then be up to the developer to handle such a case.

I understand the rarity of this response but I feel it's something than needs to be available up front instead of something handled like an issue (which it isn't).

What are your thoughts?

@kean
Copy link
Owner

kean commented Sep 13, 2022

That's a good idea. It will allow the developers to cover this scenario. I think that's what Alamofire does by default as well.

But I'm not sure throwing an error is an ideal option. It'll be nice to allow developers to use optionals and consider this scenario a successful completion. I was thinking something like this:

protocol OptionalProtocol {}

extension Optional: OptionalProtocol {}

func decodeResponse<T: Decodable>(_ type: T.Type) {
    print(type is OptionalProtocol.Type)
}

@Pomanks
Copy link
Contributor Author

Pomanks commented Sep 13, 2022

It's not indeed, I just didn't see any other option at the time 😅
I get it now.

I'll make a PR 👍🏻

@Pomanks
Copy link
Contributor Author

Pomanks commented Sep 14, 2022

Here you go: Pull Request #58

@kean
Copy link
Owner

kean commented Sep 17, 2022

Released in 2.1.0. Thanks for your contribution, @Pomanks!

@kean kean closed this as completed Sep 17, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants