Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow errors to send an error code in the response extension #246

Merged
merged 3 commits into from
Mar 11, 2020

Conversation

DJLemkes
Copy link
Contributor

@DJLemkes DJLemkes commented Feb 27, 2020

A first attempt to allow errors to define an error code which is renderd in the extensions section of error elements. Closely related to #158

I have no idea whether this is solution is on the right track. If not, no hard feelings on rejections :). If it is on the right track, I can put in a little more effort to include more error information as mentioned in #158

Closes #158

@ghostdogpr
Copy link
Owner

@DJLemkes thanks for volunteering on this! I think the idea of using a trait extending Throwable works. But I would make the function return a Option[ObjectValue] (the same type we used for extensions in GraphQLResponse). That way, users are able to return a Map containing anything they want, instead of us forcing them to return something specific (extensions have no specified fields, anything could be used).

I had another approach in mind, let me explain it and you can tell me what you think. If we added Option[ObjectValue] (or maybe directly ObjectValue) directly in CalibanError (and each of the 3 case classes), users would be able to use mapError on the interpreter and put anything they want into it without having to use a custom Throwable and modify their business code. They could easily remove the items Caliban adds (e.g. explanation text for validation errors) or add their own based on the type of innerThrowable which could be their own business error type. What do you think?

Tip: run fmt in sbt to format sources and allow the CI to run the tests

@DJLemkes
Copy link
Contributor Author

DJLemkes commented Mar 1, 2020

@ghostdogpr Like your idea better! Will give it a shot in the next couple of days.

@DJLemkes
Copy link
Contributor Author

DJLemkes commented Mar 6, 2020

@ghostdogpr sorry, had the flue hence the delay. Could you have a look and tell me if this is what you had in mind?

@ghostdogpr
Copy link
Owner

@DJLemkes no problem, I'll have a closer look tomorrow!

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just have some minor remarks.

Json
.obj(
"message" -> s"Parsing Error: $msg".asJson,
"locations" -> Some(locationInfo).collect {
case Some(li) => Json.arr(locationToJson(li))
}.asJson
}.asJson,
"extensions" -> extensions.asInstanceOf[Option[ResponseValue]].asJson.dropNullValues
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(extensions: Option[ResponseValue]) would be better than asInstanceOf (here and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot of sense but I can't seem to get the encoder for an Option[ResponseValue] in implicit scope such that .asJson works. Thought that the implicit circeEncoder of ResponseValue would be able to pick this one up? I got the .asInstanceOf[ResponseValue] trick from GraphQLResponse actually.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(extensions: Option[ResponseValue]).asJson.dropNullValues doesn't work? We were about to remove the asInstanceOf in this PR by using that same trick: https://github.com/ghostdogpr/caliban/pull/234/files#diff-16ab822f0162e843af921d1e9d0c34dfR23

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I was trying to put it in the match clause. Apparently didn't fully understand what you meant. Changed it now and it works!

examples/src/main/scala/caliban/http4s/ExampleApp.scala Outdated Show resolved Hide resolved
Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Json
.obj(
"message" -> s"Parsing Error: $msg".asJson,
"locations" -> Some(locationInfo).collect {
case Some(li) => Json.arr(locationToJson(li))
}.asJson
}.asJson,
"extensions" -> extensions.asInstanceOf[Option[ResponseValue]].asJson.dropNullValues
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(extensions: Option[ResponseValue]).asJson.dropNullValues doesn't work? We were about to remove the asInstanceOf in this PR by using that same trick: https://github.com/ghostdogpr/caliban/pull/234/files#diff-16ab822f0162e843af921d1e9d0c34dfR23


## Customizing error responses

During various phases of executing a query, an error may occur. Caliban renders the different instances of `CalibanError` to a GraphQL spec compliant response. As a user, you will most likely encounter `ExecutionError` at some point because this will encapsulate the errors in the error channel of your effects. For Caliban to be able to render some basic message about the error that occured during query execution, it is important that your errror extends `Throwable`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: errror

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.


During various phases of executing a query, an error may occur. Caliban renders the different instances of `CalibanError` to a GraphQL spec compliant response. As a user, you will most likely encounter `ExecutionError` at some point because this will encapsulate the errors in the error channel of your effects. For Caliban to be able to render some basic message about the error that occured during query execution, it is important that your errror extends `Throwable`.

For more meaningful error handling, GraphQL spec allows for an [`extension`](http://spec.graphql.org/June2018/#example-fce18) object in the error response. This object may include, for instance, `code` information to model enum-like error codes that can be handled by a front-end. In order to generate this information, one can use the `mapError` function on a `GraphQLInterpreter`. An example is provided below in which we map a custom domain error within an `ExecutionError` to a meaningful error code.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice doc!

@ghostdogpr ghostdogpr merged commit 7e82950 into ghostdogpr:master Mar 11, 2020
@ghostdogpr
Copy link
Owner

@DJLemkes thanks for your contribution!

@DJLemkes
Copy link
Contributor Author

@ghostdogpr Thanks for you reviews and patience! And for creating the library of course :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors: support extensions
2 participants