kean / Get

Web API client built using async/await
MIT License
937 stars 74 forks source link

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

Closed Pomanks closed 2 years ago

Pomanks commented 2 years ago

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).

kean commented 2 years ago

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.

Pomanks commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Here you go: Pull Request #58

kean commented 2 years ago

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