Graphql_lwt: Return errors as JSON object #96
Conversation
| @@ -34,7 +34,7 @@ module Server = struct | |||
| C.Server.respond_string ~status:`OK ~body () | |||
| | Error err -> | |||
| let body = Yojson.Basic.to_string err in | |||
| C.Server.respond_error ~body () | |||
| C.Server.respond_string ~status:`Internal_server_error ~body () | |||
There was a problem hiding this comment.
should these also add a Content-Type header?
There was a problem hiding this comment.
That would be nice, but I'm fine leaving that for a separate PR.
| @@ -34,7 +34,7 @@ module Server = struct | |||
| C.Server.respond_string ~status:`OK ~body () | |||
| | Error err -> | |||
| let body = Yojson.Basic.to_string err in | |||
| C.Server.respond_error ~body () | |||
| C.Server.respond_string ~status:`Internal_server_error ~body () | |||
There was a problem hiding this comment.
That would be nice, but I'm fine leaving that for a separate PR.
graphql-lwt/test/lwt_test.ml
Outdated
| "errors", `List [ | ||
| `Assoc [ "message", `String "error" ] | ||
| ] | ||
| ])) |
There was a problem hiding this comment.
I'm not sure this test case adds much beyond what's already covered in error_test.ml.
It would be great to start a new suite of tests for Graphql_lwt.Server though, which is currently untested. I don't consider that a requirement for merging this PR though.
There was a problem hiding this comment.
Since this bug happened in graphql-lwt, I thought it would be nice to add a test that checks if errors are returned as objects. Thanks for the pointer to the error_test module, I missed that it does test this exact behavior. I'll remove the test from the commit.
ozanmakes
force-pushed
the
return_json_errors
branch
from
dddb241 to
fa103a6
Compare
ozanmakes
force-pushed
the
return_json_errors
branch
from
fa103a6 to
39b8c8c
Compare
|
Thanks! |
As discussed in #95