Error policies, huge refactors, and a lot of other breaking changes #480
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
toddburnside
approved these changes
Mar 6, 2023
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| package clue.data | |||
|
|
|||
| import io.circe.Json | |||
| // import io.circe.Json | |||
| } | ||
|
|
||
| protected def subscribeInternal[D: Decoder]( | ||
| // def subscribe_( |
| @@ -118,16 +127,16 @@ object Demo extends IOApp.Simple { | |||
| withLogger[IO].use { implicit logger => | |||
| withStreamingClient[IO].use { implicit client => | |||
| for { | |||
| result <- client.request(Query) | |||
| result <- client.request(Query) // (ErrorPolicyInfo.RaiseAlwaysInfo) | |||
armanbilge
reviewed
Mar 6, 2023
| } | ||
|
|
||
| sealed trait ErrorPolicyProcessor[D, R] { | ||
| def process[F[_]: Sync](result: GraphQLCombinedResponse[D]): F[R] |
There was a problem hiding this comment.
Why does this need Sync? Can the delays below just be pure, in which case ApplicativeThrow is all that is needed?
There was a problem hiding this comment.
That sounds right, good catch too.
Comment on lines
14
to
12
| class ConnectionException() extends GraphQLException("Could not establish connection") | ||
| case object ConnectionException extends GraphQLException("Could not establish connection") | ||
|
|
||
| class DisconnectedException() extends GraphQLException("Connection was closed") | ||
| case object DisconnectedException extends GraphQLException("Connection was closed") |
There was a problem hiding this comment.
Exceptions that are case objects can extend scala.util.control.NoStackTrace since their stack trace would be confusing since it's not related to the error.
There was a problem hiding this comment.
Ah good catch, maybe they should't be objects then, stack traces may be useful.
armanbilge
approved these changes
Mar 7, 2023
This was referenced Mar 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR address #475, #476 and #477, and partially #341.
Regarding the handling of the case when both
dataanderrors(#476), the solution could easily be extended to address (optionally) not stopping a subscription on error (#475).The solution I sought aimed to allow
data+errorshandling, but allow users to ignore this cases, raising all errors if desired and simplifying the interface when the invoked service is known not to return these cases. I think it's simpler to deal with anF[Data]than with anF[Ior[GraphQLErrors, Data]].The approach taken was to add a new
ErrorPolicyimplicit parameter to all queries and subscriptions. This determines how to handle errors and therefore the return types of queries and subscriptions.Return types are determined via path-dependent types. This results in the caveat that the
ErrorPolicymust be statically known at the call sites for the compiler to be able to solve the return type into a useful type.For example,
ErrorPolicy.RaiseAlwaysdetermines that a query return type isF[Data]. The compiler won't be able to solve this if theErrorPolicywas passed as a parameter to the a method from where the query is made. In practice, this means that we can have a global error policy and import it everywhere queries and subscriptions are made (or pass it explicitly).Regarding extensions (#477): they are now supported in the protocol, paving the way for clients that makes use of it.
Regarding spec compliance (#341): I've reached the conclusion that's it's neither easy nor trivial to pursue modeling the base GraphQL specification. We should still seek to adhere to the HTTP and WS specs. There are number of changes towards that goal, but there's still work to do, especially untangling the WS protocol from the connection and reconnection logic.
A number of other refactors were made to better adjust types.
Changes to documentation were done by "search and replace", the documentation is still heavily outdated.
The
Demowas updated and now it runs again (at least as long as we have the oldlucuma-odbdevelopment server up).