Add tapErrorZIO and tapErrorCauseZIO to Route and Routes #2755

tjarvstrand · 2024-03-30T16:50:01Z

Useful for things like error logging or reporting to external services such as Sentry.

codecov-commenter · 2024-03-30T16:55:38Z

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 64.34%. Comparing base (709b459) to head (006b320).

Files	Patch %	Lines
...io-http/shared/src/main/scala/zio/http/Route.scala	0.00%	8 Missing ⚠️
...o-http/shared/src/main/scala/zio/http/Routes.scala	0.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2755      +/-   ##
==========================================
- Coverage   64.42%   64.34%   -0.08%     
==========================================
  Files         148      148              
  Lines        8668     8678      +10     
  Branches     1589     1545      -44     
==========================================
  Hits         5584     5584              
- Misses       3084     3094      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

987Nabil · 2024-04-08T04:32:52Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+    self match {
+      case Provided(route, env)                        => Provided(route.tapErrorCauseZIO(f), env)
+      case Augmented(route, aspect)                    => Augmented(route.tapErrorCauseZIO(f), aspect)
+      case Handled(routePattern, handler, location)    => Handled(routePattern, handler, location)


iirc, ensures handled that the typed error is handled. But a cause (defect) might still happen. So I think there should be a tap on the handler here.

That doesn't seem to work as the Handled.handler's failure type is Response whereas f's failure type is Err which is not necessarily a Response.

You can catch the Cause[Err], and if it contains an Err, then you rethrow it. But if it's really a Cause[Nothing], i.e. pure defect, without an Err inside it, then you can allow that to be caught by f, since f can handle those (Cause[Nothing] is a subtype of Cause[Err]).

@tjarvstrand This does need to be fixed, and then will be good to merge!

Sorry, busy week! :/

I've been struggling a bit with the error case of f which, unless I'm mistaken, must be a Response. Does something like this make sense?

/** * Effectfully peeks at the unhandled failure cause of this Route. */ final def tapErrorCauseZIO[Err1 >: Err]( f: Cause[Err] => ZIO[Any, Err1, Any], )(implicit trace: Trace): Route[Env, Err1] = self match { case Provided(route, env) => Provided(route.tapErrorCauseZIO(f), env) case Augmented(route, aspect) => Augmented(route.tapErrorCauseZIO(f), aspect) case Handled(routePattern, handler, location) => Handled( routePattern, handler.tapErrorCauseZIO { case cause0: Cause[Err] => f(cause0).catchAllCause(cause => ZIO.fail(Response.fromCause(cause))) case _ => ZIO.unit }, location, ) case Unhandled(rpm, handler, zippable, location) => Unhandled(rpm, handler.tapErrorCauseZIO(f), zippable, location) }

@tjarvstrand What I meant originally was we don't need pattern matching to determine if it's Cause[Nothing] or not, according to John's previous comment.

You can catch the Cause[Err], and if it contains an Err, then you rethrow it. But if it's really a Cause[Nothing], i.e. pure defect, without an Err inside it, then you can allow that to be caught by f, since f can handle those (Cause[Nothing] is a subtype of Cause[Err]).

But now we want to let f handle failure cases as well when Err =:= Response, right?

I think there are two options:

~~Extract the possible failure with something like Cause#failureOrCause, and do runtime type check for the failure case using .isInstanceOf[Response]. It's the simplest solution.~~ EDIT: It seems irrelevant.

Require an implicit zio.Tag[Err] or scala.reflect.ClassTag[Err] and check its runtime class.

Add an implicit evidence like this:

sealed trait IsResponse[+A] { def isResponse: Boolean } object IsResponse { implicit val response: IsResponse[Response] = new IsResponse[Response] { isResponse = true } // to avoid unnecessary allocation private val otherInstance: IsResponse[Nothing] = new IsResponse[Nothing] { isResponse = false } implicit def other: IsResponse[A] = otherInstance }

The name IsResponse might cause confusion with Handler.IsRequest, which has the similar purpose but it's for compile time check only.

By the way, Handled.handler is a handler that produces another handler (code), so checking handler's failure won't achieve what you want. You need handler.map(_.tapErrorCauseZIO(...)) to catch the Response failure.

Thanks a lot for your comments!

If I understand it correctly, both alternative 2 and 3 would require a call to asInstanceOf, since neither of them provide a type level proof that Err <: Response. Is that correct?

By the way, Handled.handler is a handler that produces another handler (code), so checking handler's failure won't achieve what you want. You need handler.map(_.tapErrorCauseZIO(...)) to catch the Response failure.

Aha, I missed that! Thanks!

Alternative 2 sadly doesn't work since ClassTag is invariant: Covariant type Err occurs in invariant position in type ClassTag[Err] of value ct.

I gave alternative 3 a shot instead, but I had to make it contravariant rather than covariant.

You can't directly summon ClassTag[Err] because it's invariant. Instead, introduce a new type variable like Err1.

Check out ZIO#refinedToOrDie for an example:

def refineToOrDie[E1 <: E: ClassTag](implicit ev: CanFail[E], trace: Trace): ZIO[R, E1, A] = self.refineOrDie { case e: E1 => e }

guersam · 2024-07-03T09:54:06Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+        Handled(
+          routePattern,
+          handler.map(_.tapErrorCauseZIO {
+            case err: Fail[_] if ev.isResponse =>


If ev.IsResponse == false we don't need handler.map(...) at all.

guersam · 2024-07-03T09:54:41Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+          routePattern,
+          handler.map(_.tapErrorCauseZIO {
+            case cause0 if ev.isResponse =>
+              f(cause0.asInstanceOf[Cause[Err]]).catchAllCause(cause => ZIO.fail(Response.fromCause(cause)))


If the cause is not a failure, it can be passed to f even if Err =!= Response as John pointed out in the previous comment I quoted.

Ah, yeah, sorry I misread that one!

Same here about the .catchAllCause

guersam · 2024-07-03T10:01:36Z

zio-http/shared/src/main/scala/zio/http/Route.scala

@@ -489,4 +537,16 @@ object Route                   {
    }
  }

+  sealed trait IsResponseCauseHandler[-A] { def isResponse: Boolean }


The most type evidences starting with Is tend provide the implicit instance only and if only the type.

As it always provides the implicit value, maybe we need another name to avoid confusion.

Possible alternatives:

DetermineResponse

CheckResponse

ResponseTypeChecker

...

I think CauseHandler is somewhat misleading as this type is not directly related to handler.

Good point. Changing it to CheckResponse

tjarvstrand · 2024-07-04T11:25:57Z

Thanks for the feedback!

guersam · 2024-07-04T12:49:48Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+          routePattern,
+          if (ev.isResponse) {
+            handler.map(_.tapErrorCauseZIO {
+              case err: Fail[_] if ev.isResponse =>


Now we don't need this pattern guard :)

guersam · 2024-07-05T03:41:00Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+          if (ev.isResponse) {
+            handler.map(_.tapErrorCauseZIO {
+              case err: Fail[_] if ev.isResponse =>
+                f(err.value.asInstanceOf[Err]).catchAllCause(cause => ZIO.fail(Response.fromCause(cause)))


Now I doubt this .catchAllCause(cause => ZIO.fail(Response.fromCause(cause))) is necessary, because it may cause unexpected behavior when there is a global error handler, say catching all defects and produces a formatted error response.

As we're sure that Err =:= Response, just '.asInstance[...]' can be enough.

I think we do need the catchAllCause since we don't know that Err1 <: Response

guersam · 2024-07-05T03:42:16Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+          routePattern,
+          handler.map(_.tapErrorCauseZIO {
+            case cause0 if ev.isResponse =>
+              f(cause0.asInstanceOf[Cause[Err]]).catchAllCause(cause => ZIO.fail(Response.fromCause(cause)))


Same here about the .catchAllCause

guersam · 2024-07-05T03:43:07Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+            case _: Fail[_]              =>
+              ZIO.unit
+            case cause0                  =>
+              f(cause0.asInstanceOf[Cause[Nothing]]).catchAllCause(cause => ZIO.fail(Response.fromCause(cause)))


Same here about the .catchAllCause.

987Nabil · 2024-08-27T21:12:50Z

@tjarvstrand Could you handle the review comments? I'd like to get this in for 3.0

tjarvstrand · 2024-08-31T06:39:40Z

The tests seem to fail on something unrelated to my changes. Not sure if it's a fluke but I don't seem to be allowed to restart the workflow.

987Nabil · 2024-08-31T17:13:07Z

I updated your branch. Let's see if it builds

987Nabil · 2024-09-05T10:07:30Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+      case Handled(routePattern, handler, location)    =>
+        Handled(
+          routePattern,
+          if (ev.isResponse) {


This is always true. Handled has a fixed error type of Nothing and will pick up the implicit val response: CheckResponse[Response]

Handled Routes are by definition routes without error. This should just returned the route without any modification.
The becomes clear when substituting the types. Because f is Nothing => ZIO[Any, Err1, Any]

987Nabil · 2024-09-05T10:13:16Z

zio-http/shared/src/main/scala/zio/http/Route.scala

+        Handled(
+          routePattern,
+          handler.map(_.tapErrorCauseZIO {
+            case cause0 if ev.isResponse =>


ev.isResponse is always true. Also the concrete type of f here is Cause[Nothing] => ZIO[Any, Err1, Any]

Having f as Cause[Nothing] => ZIO[Any, Err1, Any] doesn't seem to work as it makes the call to handler.tapErrorCauseZIO in the Unhandled-case sad.

987Nabil · 2024-09-05T10:20:46Z

Please add some tests. For example adding a log output and use the test logger to find the message you logged. Or use a promise. Or whatever comes to your mind :)

987Nabil · 2024-12-23T05:38:52Z

@tjarvstrand could you give this PR the finishing touch? :)

tjarvstrand · 2024-12-30T20:47:55Z

Looking into writing some tests ATM. In the meantime, I think I'm lacking the context needed to make wise decisions on my own here 😄

Just to make sure I don't misunderstand, you are saying that I should basically just return the same Handled value on both what is currently line 172 and 197? E.g.:

  final def tapErrorCauseZIO[Err1 >: Err](
    f: Cause[Err] => ZIO[Any, Err1, Any],
  )(implicit trace: Trace, ev: CheckResponse[Err]): Route[Env, Err1] =
    self match {
      case Provided(route, env)                        => Provided(route.tapErrorCauseZIO(f), env)
      case Augmented(route, aspect)                    => Augmented(route.tapErrorCauseZIO(f), aspect)
      case handled @ Handled(_, _, _)                  => handled
      case Unhandled(rpm, handler, zippable, location) =>
        Unhandled(rpm, handler.tapErrorCauseZIO(f), zippable, location)
    }

EDIT: I think I figured most of it out. Everything became clearer once my mind got back into "scala mode". Please have another look.

However, I noticed that the handler of an Unhandled doesn't seem to be evaluated. Should that case also just return the same value?

tjarvstrand requested review from jdegoes, vigoo, adamgfraser and 987Nabil as code owners March 30, 2024 16:50

987Nabil reviewed Apr 8, 2024

View reviewed changes

tjarvstrand force-pushed the routes-tap-error-cause-zio branch from 006b320 to 1194943 Compare April 17, 2024 14:40

tjarvstrand force-pushed the routes-tap-error-cause-zio branch 2 times, most recently from 1f6c5e0 to 5589573 Compare July 3, 2024 08:55

guersam reviewed Jul 3, 2024

View reviewed changes

tjarvstrand force-pushed the routes-tap-error-cause-zio branch 3 times, most recently from cd742b1 to 933ec35 Compare July 4, 2024 11:32

guersam reviewed Jul 5, 2024

View reviewed changes

tjarvstrand force-pushed the routes-tap-error-cause-zio branch 2 times, most recently from 5a700da to c45935a Compare August 30, 2024 15:07

987Nabil requested changes Sep 5, 2024

View reviewed changes

tjarvstrand force-pushed the routes-tap-error-cause-zio branch from 64337ba to ed54097 Compare December 30, 2024 21:38

Add tapErrorZIO and tapErrorCauseZIO to Route and Routes

255f105

tjarvstrand force-pushed the routes-tap-error-cause-zio branch from ed54097 to 255f105 Compare December 30, 2024 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tapErrorZIO and tapErrorCauseZIO to Route and Routes #2755

Add tapErrorZIO and tapErrorCauseZIO to Route and Routes #2755

tjarvstrand commented Mar 30, 2024 •

edited

Loading

codecov-commenter commented Mar 30, 2024 •

edited

Loading

987Nabil Apr 8, 2024

tjarvstrand Apr 8, 2024 •

edited

Loading

jdegoes Apr 8, 2024

jdegoes Apr 16, 2024

tjarvstrand Apr 16, 2024

guersam Jul 3, 2024 •

edited

Loading

guersam Jul 3, 2024 •

edited

Loading

tjarvstrand Jul 3, 2024

tjarvstrand Jul 3, 2024

guersam Jul 4, 2024 •

edited

Loading

guersam Jul 3, 2024

tjarvstrand Jul 4, 2024

guersam Jul 3, 2024 •

edited

Loading

tjarvstrand Jul 4, 2024

guersam Jul 5, 2024

guersam Jul 3, 2024

tjarvstrand Jul 4, 2024

tjarvstrand commented Jul 4, 2024

guersam Jul 4, 2024

tjarvstrand Aug 30, 2024

guersam Jul 5, 2024

tjarvstrand Aug 30, 2024

guersam Jul 5, 2024

guersam Jul 5, 2024

987Nabil commented Aug 27, 2024

tjarvstrand commented Aug 31, 2024

987Nabil commented Aug 31, 2024

987Nabil Sep 5, 2024

987Nabil Sep 5, 2024

tjarvstrand Dec 30, 2024

987Nabil commented Sep 5, 2024

987Nabil commented Dec 23, 2024

tjarvstrand commented Dec 30, 2024 •

edited

Loading

Add tapErrorZIO and tapErrorCauseZIO to Route and Routes #2755

Are you sure you want to change the base?

Add tapErrorZIO and tapErrorCauseZIO to Route and Routes #2755

Conversation

tjarvstrand commented Mar 30, 2024 • edited Loading

codecov-commenter commented Mar 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tjarvstrand Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guersam Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

guersam Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guersam Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guersam Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjarvstrand commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

987Nabil commented Aug 27, 2024

tjarvstrand commented Aug 31, 2024

987Nabil commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

987Nabil commented Sep 5, 2024

987Nabil commented Dec 23, 2024

tjarvstrand commented Dec 30, 2024 • edited Loading

tjarvstrand commented Mar 30, 2024 •

edited

Loading

codecov-commenter commented Mar 30, 2024 •

edited

Loading

tjarvstrand Apr 8, 2024 •

edited

Loading

guersam Jul 3, 2024 •

edited

Loading

guersam Jul 3, 2024 •

edited

Loading

guersam Jul 4, 2024 •

edited

Loading

guersam Jul 3, 2024 •

edited

Loading

tjarvstrand commented Dec 30, 2024 •

edited

Loading