Work Publisher.decode with JSONDecoder is it a bug or a feature?

The following code will not work:

struct Repo: Codable, CustomStringConvertible {
    let full_name: String
    var description: String { return full_name }
}

let url = URL(string: "https://api.github.com/repositories")!
var request = URLRequest(url: url)
request.httpMethod = "GET"
request.setValue("application/json", forHTTPHeaderField: "Content-Type")

URLSession.shared.dataTaskPublisher(for: request)
    .decode(type: [Repo].self, decoder: JSONDecoder())
    .sink { (repos) in
        print("\(repos)")
    }

Error in decode method:

Instance method 'decode(type:decoder:)' requires the types 'URLSession.DataTaskPublisher.Output' (aka '(data: Data, response: URLResponse)') and 'Data' be equivalent

I wrote an extension that will fix it:

@available(OSX 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *)
extension Publisher where Output == URLSession.DataTaskPublisher.Output {
    public func decode<Item, Coder>(type: Item.Type, decoder: Coder) -> Publishers.TryMap<Self, Item> where Item: Decodable, Coder: TopLevelDecoder, Coder.Input == Data {
        return tryMap { try decoder.decode(type, from: $0.data) }
    }
}

I attach a link to gist. There's also a more detailed description and other examples of how to solve the problem.

Can I something do not understand? And this is how it was originally conceived :slight_smile:
But then there is a conflict with the description in the documentation.

1 Like

I don't think there's a problem here. URLSession's dataTaskPublisher returns a tuple of (Data, URLResponse), and decode operator expects "Data" as its input. In order to connect operator to a publisher, operator's input type should match publisher's output.
So you need to extract data with something like this:

URLSession.shared.dataTaskPublisher(for: request)
     .map { $0.data }
     .decode(type: [Repo].self, decoder: JSONDecoder())
3 Likes

I think it is a problem. Why I always need to add an extension to project, when it's can be default in the system?

1 Like

Thank you, but I know this way. I described it in the attached gist.

Always write .map { $0.data } doesn't look good. In that case, why does the decode method exist? When it can't be used without crutches.

The decode method is intended to be used on Publishers that output Data, the URLSession.DataTaskPublisher does not output Data. While it might seem inconvenient that for every URL request you want to decode you have to add that map { $0.data } line, I think it makes sense. The URLSession.DataTaskPublisher should not output Data directly because you might need to access the URLResponse. The decode method should not expect a tuple of (Data, URLResponse) because the decode should be usable outside of the scope of URLSession.

So IMO, it's not a problem and a default in the system wouldn't make a lot of sense.

Ps. You mentioned a conflict in the documentation, I'm not sure I see what the conflict is actually.

2 Likes

The decode method is intended to be used on Publishers that output Data

I agree, but in this case, this method should be explicitly stated that it only works with Data.
And so it turns out that we can call from anywhere and then flop with crutches. A small amount of code can easily solve the problem, but in production it can be a very serious problem given the huge number of encapsulations.

Looking at the documentation, it looks like the decode method is a bit more generic than just being able to work with data. It seems that you can use any TopLevelDecoder which has an Input type associated with it. For JSONDecoder that Input would be data. In the function signature you can see that Coder.Input must equal Self.Output. The documentation could possibly be a bit more clear on what Self is here, but I assume it's the "previous" Publisher. In this case one where the Output would be (Data, URLResponse) which is not equal to the Data input JSONDecoder expects.

To make the decoder method work as you describe I see two options:

  1. make decoder accept (Data, URLResponse) and then automatically extract Data and decode that.
  2. make URLSession.DataTaskPublisher's Output a Data object so it produces a valid input for the decoder
  3. Overload decoder so it accepts both (Data, URLResponse) and Data

All solutions would create a tight coupling between the data task publisher and the decoder method, which is undesirable.
Argument against solution 1: You might not always want to use decoder as result of a network request.
Argument against solution 2: Sometimes you need access to a URLResponse / you don't always want to use the decode method on data returned from a webserver.
Argument against solution 3: This would add unneeded bloat to the standard library IMO and would imply the possibility of several others, similar overloads being added.

I don't see how extracting data before passing it to the decoder is a very serious problem, it's simply part of how you move from a network request to a decoded instance.

What if the network request fails and you receive an HTTP error code instead?

In this case, DataTaskPublisher will not fail with error, it will send you (data, response), but the body of the request (stored in data) may be unexpected, so you may want to decide what to do next based on the HTTP status code. For example, throw an error. This is roughly what I usually do in my projects:

URLSession
    .shared
    .dataTaskPublisher(for: URL(string: "https://swift.org/whatever")!)
    .tryMap { (data, response) -> Data in

        let response = response as! HTTPURLResponse

        if response.statusCode == 200 {
            return data
        }

        let localizedStatusCodeDescription =
            HTTPURLResponse.localizedString(forStatusCode: response.statusCode)

        throw MyError(description: localizedStatusCodeDescription)
    }.decode(type: MyResponse.self, decoder: JSONDecoder())

I don't think that relying on the fact that your server will never send you a 500 Internal Server Error is a good thing, because it sure as hell will at some point.

Adding such an extension to Foundation would be a bad thing precisely because of that.

3 Likes