sean7512 / RestEssentials

Lightweight REST and JSON library for Swift.
MIT License
37 stars 12 forks source link

Better exception message when throwing MalformedResponse #17

Closed vitorhugods closed 6 years ago

vitorhugods commented 6 years ago

It is very hard to find where is the malformed part of a response when dealing with a big JSON. Would be nice if a more specific message was shown, like "Could not find "key" in the JSON".

And also, I couldn't find a way to get the raw text response and print it to help me debug.

Thanks for the library. I really wish I could be of more help, but I'm learning Swift.

sean7512 commented 6 years ago

The malformed error means different things depending on which Deserializer you are using (the old JSON or the Swift4 Codable way for JSON).

Can you post some sample code?

sean7512 commented 6 years ago

As for getting the raw Data object:

do {
    let response = try result.value()
    // ...
} catch NetworkingError.malformedResponse(let data) {
    print("Error performing GET, malformed data response: \(data!)")
}

The data variable will be of type Data? which represents the raw bytes received. In a future RestEssentials release, it will not be an optional, but just a Data object. For now, you can safely force unwrap it (only for malformedResponse errors). It is not safe to force unwrap the data object for ANY other type of error.

vitorhugods commented 6 years ago

First of all, thanks for such a quick answer!

After some source code digging, I found out that the Exception has the data, but I didn't know how to get it. Now at least I can actually see the received JSON and send reports of failure to the API I'm using.

Thanks a lot!

About the Deserializer, I'm using only Swift 4's Codable for now. Some example:

    //Usage in my ViewController
    ServerClient.fetchStartupData(){ startupdata, httpResponse in
            if let data = startupdata{
                //Handle received data
            }else{
                print("Nope, Connection Error")
            }
        }

    //Expected response
    public struct StartupDataResult: Codable {
        public let name: String
        public let photo: String
        public let active_orders: [ActiveOrder]
        public let notifications: [Notification]?
    }

    //Global function that can be accessed in my whole project
    public static func fetchStartupData(callback: @escaping (StartupDataResult?, HTTPURLResponse?) -> ()) {
        let name = "buscaDadosDeStartup"
        let dados = Server.getBasicInfo() //Codable with API Token and more basic info, nothing much
        Server.client(callName: name).post(dados, responseType: StartupDataResult.self) { result, httpResponse in
            handleDefaultConnection(callback, result, httpResponse)
        }
    }

    //Some connections only require "Data or No Data", so I created this handler
    static func handleDefaultConnection<T>(_ callback: (T?, HTTPURLResponse?) -> (), _ result: Result<T>, _ response: HTTPURLResponse?) {
        do {
            print("Response = \(response)")
            let response = try result.value()
            callback(response, response)
        } catch {
            print("Error receiving fetchStartupData = \(error)") //I will modify this go get the raw data now
            callback(nil, response)
        }
    }

Probably there is a better way to do what I'm doing, but I'm already very happy for now.

Is there any way to get an exception message that indicates what key is missing in the JSON response, or something like that, that narrows my search for API misbehavior? I've searched for JSONDecoder errors and I think those would be great tools to have.

sean7512 commented 6 years ago

It should already be logging the error that comes straight from the Swift API. If you look at the. DecodableDeserializer in Deserializer.swift you can see the error from the native JSONDecoder should be logged out:

print("Error decoding to JSON object \(error.localizedDescription)")

I am not sure how detailed the built-in decoder's errors are, but take a look there. You can breakpoint in there and see if there are different details in the error that isn't being logged out. If you find something else that I should log out, let me know.

sean7512 commented 6 years ago

@vitorhugods Take a look at the last commit: https://github.com/sean7512/RestEssentials/commit/43b15f21f61fd3db05c912ef7263a4e6daae2b54

I changed the Deserializers so they no longer swallow error, but instead pass them back up. Now when you try to get the result.value() you will get the original error back in the failure case. So you should now be getting the JSONDecoder errors that you linked to. This will be in the next release (4.0.2) and I will release it once you confirm it looks good.

Thanks!

vitorhugods commented 6 years ago

Seems really good to me. Really liked it. =)

Also, I've been thinking here about debugging and reports. In Android I normally use Firebase/Crashlytics to send reports of connection, failures or edge-case. It would be very nice to access the raw response data even successful cases as well. Is this an issue-material?

sean7512 commented 6 years ago

Ok, I'll start getting 4.0.2 released soon.

As for Firebase/Crashyltics, that would be integrated at the app level, not in the library.

Thanks for the suggestion on the error improvements!

vitorhugods commented 6 years ago

I'm not saying it should not be in the app level. But so far, the way to get the raw response body in order to log or report, without deserializing is when an serializing error occurs, as seen here. Or is there any other way to get that?

sean7512 commented 6 years ago

4.0.2 has been released and contains better error handling for Deserializers and it also removes the optional on the Data parameter for a malformedResponse error. I have also updated the Readme to include information on Error Handling.

As of now, there is no way to get the raw data back in a success case. There are some memory constraints with doing this. Imagine using an Image Deserializer on very large raw images, you would be using twice as much memory, which is probably not desirable. Or worse yet, maybe you used a Movie Deserializer.

If you use the built-in JSON type (instead of Codable), we do keep the raw response (it isn't accessible right now, it is private), but if there was a strong argument for it, I could expose it. The issue with exposing it is mutability and how that may break the json parsing. I guess it could be immutable getter only. Either way, feels awkward for only 1 of the deserializers to provide that functionality.

vitorhugods commented 6 years ago

Understood. I've never though of images and videos. I've worked in projects that some operations were really sensitive, complexes and cost big money. To the if any error was reported customer-wise, admins should be able to look logs back and see exactly what the app requested, and what it received from the API, so It would be fixed ASAP. It never came to the point that a costumer reported something, mostly because even when in beta testing, these reports helped to correct mistakes.

sean7512 commented 6 years ago

@vitorhugods going to close now that the 4.0.2 release include much better error handling. Thanks a ton for bringing the issue to my attention.