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

Reworked error handling #378

Merged
merged 3 commits into from
May 9, 2023
Merged

Reworked error handling #378

merged 3 commits into from
May 9, 2023

Conversation

milessabin
Copy link
Member

@milessabin milessabin commented May 8, 2023

  • Result is now a first class type which can represent a Success (a result of type T), a Warning (a result and also client-reportable GraphQL errors), a Failure (client-reportable GraphQL errors only) and an InternalError (an error which should not be reported to clients). Result has instances for MonadError, Traverse, Eq and Semigroup and is accompanied by a ResultT transformer.

    This is a breaking change, but its main manifestation in user code is that elaborators and effects need to use Result's .success syntax instead of Ior's .rightIor syntax.

  • Mapping's effect now requires a MonadThrow instance rather than a Monad. This is to allow internal errors in a final Result to be unpacked into the effect. This allows us to return an F[Json] or a Stream[F, Json] rather than an F[Result[Json]] or Stream[F, Result[Json]] which improves the user ergonomics somewhat.

    Unfortunately the use of Streams means that for the pure tests in core we can't replace Id with Either[Throwable, *] because there is no instance of fs2.Compiler for that type. Rather than jump through hoops to support pure operation, which is only likely to be useful in tests, the formerly pure tests have simply been switched over to IO.

  • Several cases where illegal states were silently ignored are now correctly reported as internal errors.

  • All failure cases have been checked to ensure that internal errors will not be leaked to clients.

  • All occurrences of sys.error have been eliminated.

  • All assertions now represent logic errors in Grackle. Whilst it's possible that these might be triggered by (for example) user errors in mappings, the implication is that these should have been caught and reported prior to hitting an assertion.

Fixes #199 and #200.

+ Result is now a first class type which can represent a Success (a
  result of type T), a Warning (a result and also client-reportable
  GraphQL errors), a Failure (client-reportable GraphQL errors only) and
  an InternalError (an error which should not be reported to clients).
  Result has instances for MonadError, Traverse, Eq and Semigroup and is
  accompanied by a ResultT transformer. This is a breaking change, but
  its main manifestation in user code is that elaborators and effects
  need to use Result's .success syntax in preference to Ior's .rightIor
  syntax.
+ Mapping's effect now requires a MonadThrow instance rather than a
  Monad. This is to allow internal errors in a final Result to be
  unpacked into the effect. This allows us to return an F[Json] or a
  Stream[F, Json] rather than an F[Result[Json]] or Stream[F,
  Result[Json]] which improves the user ergonomics somewhat.
  Unfortunately the use of Streams means that for the pure tests in core
  we can't replace Id with Either[Throwable, *] because there is no
  instance of fs2.Compiler for that type. Rather than jump through hoops
  to support pure operation, which is only likely to be useful in tests,
  the formerly pure tests have simply been switched over to IO.
+ Several cases where illegal states were silently ignored are now
  correctly reported as internal errors.
+ All failure cases have been checked to ensure that internal errors
  will not be leaked to clients.
+ All occurrences of sys.error have been eliminated.
+ All assertions now represent logic errors in Grackle. Whilst it's
  possible that these might be triggered by (for example) user errors in
  mappings, the implication is that these should have been caught and
  reported prior to hitting an assertion.
@milessabin milessabin requested a review from tpolecat May 8, 2023 19:37
@milessabin milessabin self-assigned this May 8, 2023
Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

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

👍 This is great. SqlMapping needed a lot of pushing-about but it's much improved I think. If it's not too much of as hassle I would like to see law-checking for Result, but feel free to merge and do it as a follow-up if you prefer.

modules/core/src/main/scala/result.scala Show resolved Hide resolved
modules/sql/src/main/scala/SqlMapping.scala Outdated Show resolved Hide resolved
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.

Ensure no internal errors leak out through the GraphQL error channel
2 participants