Error handling when server sends invalid json

[bsteuber]

I am calling
(http/get "http://api.icndb.com/jokes/22")
which interestingly delivers a php error message, but with a header including content-type application/json and status 200. Of course it is a server issue, but it would be nice if cljs-http could return an error here so I can handle it somehow instead of just failing.

[r0man]

@bsteuber I think this is still the best default behaviour. I think we should do the same here as clj-http. Note, that you can always, build your own custom HTTP client by choosing the needed middleware. Take a look here:
https://github.com/r0man/cljs-http/blob/master/src/cljs_http/client.cljs#L266

[roti]

In my case I get an exception thrown, which I mind a little, but my Problem is that I don't know how to catch it. try doesn't seem to work:

(defn testex []
 (go
   (let [response (<! (try (http/get "http://localhost:3000/sim/error" {:with-credentials? false})
                           (catch :default e
                             (println "error" e))))]
     (println response))))

The URL returns HTTP 500 with content-type "application/json" and not a json body.

What would be the correct approach here? Here's the stacktrace:

Uncaught SyntaxError: Unexpected token S in JSON at position 0
   at JSON.parse (<anonymous>)
   at cljs_http$util$json_decode (util.cljs?rel=1509894674732:63)
   at core.cljs:4829
   at Function.cljs.core.update_in.cljs$core$IFn$_invoke$arity$3 (core.cljs:4829)
   at cljs$core$update_in (core.cljs:4820)
   at cljs_http$client$decode_body (client.cljs?rel=1509894676467:83)
   at Function.<anonymous> (client.cljs?rel=1509894676467:181)
   at Function.cljs.core.apply.cljs$core$IFn$_invoke$arity$2 (core.cljs:3685)
   at cljs$core$apply (core.cljs:3676)
   at async.cljs?rel=1509880110795:712
cljs_http$util$json_decode @ util.cljs?rel=1509894674732:63
(anonymous) @ core.cljs:4829
cljs.core.update_in.cljs$core$IFn$_invoke$arity$3 @ core.cljs:4829
cljs$core$update_in @ core.cljs:4820
cljs_http$client$decode_body @ client.cljs?rel=1509894676467:83
(anonymous) @ client.cljs?rel=1509894676467:181
cljs.core.apply.cljs$core$IFn$_invoke$arity$2 @ core.cljs:3685
cljs$core$apply @ core.cljs:3676
(anonymous) @ async.cljs?rel=1509880110795:712
(anonymous) @ async.cljs?rel=1509880110795:702
cljs$core$async$state_machine__29062__auto____1 @ async.cljs?rel=1509880110795:702
cljs$core$async$state_machine__29062__auto__ @ async.cljs?rel=1509880110795:702
cljs$core$async$impl$ioc_helpers$run_state_machine @ ioc_helpers.cljs?rel=1509880107319:35
cljs$core$async$impl$ioc_helpers$run_state_machine_wrapped @ ioc_helpers.cljs?rel=1509880107319:39
(anonymous) @ ioc_helpers.cljs?rel=1509880107319:48
(anonymous) @ channels.cljs?rel=1509880106192:61
cljs$core$async$impl$dispatch$process_messages @ dispatch.cljs?rel=1509880105746:19
channel.port1.onmessage @ nexttick.js:211

[jmlsf]

I just hit this too. @r0man I have to say I'm confused why you think this is the best default behavior. If cljs-http gets bad json from the server, what it does right now is print a stack trace to the console, which, of course, is shorn of any context so it's impossible to tell what line of calling code even caused the problem. This is baffling since cljs-http already has an error reporting mechanism. Why not just catch exceptions and report them through the error mechanism? I also don't even begin to understand how that piece of code could help calling code. Are you suggesting that we just fork cljs-http?

[Deraen]

try-catch could be added on decode-body: https://github.com/r0man/cljs-http/blob/master/src/cljs_http/client.cljs#L103 and if decode fn throws error, add :error-code to the response, and maybe the original exception on some property.

Changing this would cause response to be returned (with empty body) when body can't be decoded.
This would break existing code, because users don't check :error-code but only :status of response, and I don't think it would make sense changing status code in this case. Not sure if this is a problem though.

[jmlsf]

I'm probably missing something, but right now, if you hit this bug, the entire state machine vaporizes and it never returns. So whatever you do, it surely has to be better than that. You can't break any calling code in this code path because (I think) it never returns to the calling code.

[Deraen]

Yes, that's the thing. Because currently response is never returned, current user code probably isn't prepared to check if the returned body was decoded successfully. Returning response without decoded body might break code which isn't prepared for these responses.

[Deraen]

Maybe it would make sense to return response with status 0, like:

{:status 0
 :success false
 :error-code :bad-response-body
 :error exception
 :original-status 200 (or whatever)
 ...}

Status code 0 would probably enable most existing code to handle these responses as error. I think e.g. Chrome uses status code 0 already in some cases (request timeout, CORS).

[r0man]

Another option would be to use something like this:

http://swannodette.github.io/2013/08/31/asynchronous-error-handling
https://github.com/alexanderkiel/async-error

Any thoughts? @jmlsf @Deraen

@jmlsf I don't have time to work on this myself at the moment, so a patch is very welcome. No need to fork this, just talk to me. I'm open to suggestions. ;)

[jmlsf]

I believe the problem is here:

(defn wrap-json-response
  "Decode application/json responses."
  [client]
  (fn [request]
    (-> #(decode-body % util/json-decode "application/json" (:request-method request))
        (async/map [(client request)]))))

The issue is that util/json-decode call js/JSON.parse, which can throw.

I think the way to fix this is to change decode-body to catch exceptions and return what @Deraen suggested like this:

(defn decode-body
  "Decocde the :body of `response` with `decode-fn` if the content type matches."
  [response decode-fn content-type request-method]
  (try
    (if (and (not= :head request-method)
             (not= 204 (:status response))
             (re-find (re-pattern (str "(?i)" (escape-special content-type)))
                      (str (clojure.core/get (:headers response) "content-type" ""))))
      (update-in response [:body] decode-fn)
      response)
    (catch e
      (merge response
             {:status 0
              :success false
              :error-code :bad-response-body
              :error e
              :original-status (:status response)}))))

The alternative is to push the exception on the channel instead of the call to merge above, but that would change the library quite a bit because callers would now be expected to use <?. From @Deraen's comments above, I assume this would not be good. Maybe I misunderstand what @r0man was suggesting?

I should point out I'm happy to try my hand at a PR, but I wanted to see if I was barking up the wrong tree first.

[r0man]

@jmlsf @Deraen Yes, I suggested going with <?, even if it is a breaking change. We can communicate this and bumb the version.

However, I'm open to anything. What kind of API would you prefer as the user of this lib? I'm asking because I actually don't use this lib much at the moment myself, and wonder if all those keys you have to check are not a bit overwhelming.

:status, :original-status, :error and :error-code are a bit too much for my taste :)

[jmlsf]

I think my two goals are (1) provide a programatic way to deal with errors and (2) make it debuggable. Dealing with 2 is fairly easy: as long as you communicate the error in some way, either by returning a status object or throwing an exception. But dealing with (1) requires that we catch exceptions where they occur in the middleware and assign and document error codes (i.e., :error-code :bad-response-body). I think that pretty much requires that we have an error/status object with a bunch of fields, because some of them might come from the network layer and some might come from the middleware.

If we were allowed to break the interface, I think the conventions around <? would be really nice because it provides an unequivocal way of knowing whether things succeeded or not: if the result is a js/Error, it's an error. Otherwise it succeeded. The calling client can use go-try and <? if they want. The implementation is pretty easy: just build the status object (if needed) by setting the properties on a js/Error object and put! it on the channel if there is an error. Otherwise, put what is currently in :body on the channel directly.

One thing that's nice about using an error object to indicate failure is that you don't have to have :original-status and :original-code. You can just leave them be and set the :error / :error-code in the middleware. :success and :status would be information about the network call.

I do like the idea of trying to isolate anything that is known to throw an error (like JSON.parse) and assigning (and documenting) an error code for it.

[r0man]

@jmlsf I think <? should return either an instance of ex-info (which could include more info via ex-data) or a complete ring response map, with {:status ... :body .. etc }, not only the value of :body.

[jmlsf]

Sorry you are right of course. At a minimum people will need the status code.

[josephhanceslm]

I have an identical issue where the server back-end is sending a VALID JSON response but is sending an HTTP 400 error response. The traceback is:

[kanaka]

I hit what I believe is the same problem when decoding EDN with an unsupported type: "No reader function for tag ordered/set."