Remove warning header; Configurable generated error responses (#2955) #3050

987Nabil · 2024-08-23T07:41:52Z

I removed the typed warning header code. The intention is, to not encourage usage of a deprecated header.
I redesigned how error messages are created if generated from Throwable through internal code and certain user calls, like sandbox. The basic idea is, that the user can auto generate error responses and based on config the details are shown or not. The config is saved in a fiber ref. That makes it possible to not only set a global default, but override config based on the route or even the request. I added for that some middleware to support this in an easy way. For example, the following code will change the config based on a set header, to get maximum details for the error.

myRoute @@ ErrorResponseConfig.debug.whenHeader(_.get("zio-debug").contains("true"))

By default, the generated error responses only have a status code.
The error response will be in HTML, JSON or plaintext, based on the accept header^

fixes #2955
fixes #2701
fixes #2948

/claim #2955
/claim #2701
/claim #2948

kyri-petrou · 2024-08-23T11:52:54Z

zio-http/jvm/src/main/scala/zio/http/netty/server/ServerInboundHandler.scala

-            attemptFastWrite(ctx, Response.fromThrowable(throwable))
+            attemptFastWrite(
+              ctx,
+              Response.fromThrowable(throwable, runtime.unsafeRunSync(ErrorResponseConfig.configRef.get)),


It seems a bit wasteful to start a fiber only to extract a FiberRef from it.

How about adding this method to NettyRuntime:

def getRef[A](ref: FiberRef[A]): A = zioRuntime.fiberRefs.getOrDefault(ref)

And then this becomes:

Response.fromThrowable(throwable, runtime.getRef(ErrorResponseConfig.configRef))

kyri-petrou · 2024-08-23T12:05:42Z

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

@@ -58,6 +58,9 @@ final case class Response(
        self.copy(body = Body.fromChunk(bytes))
      }

+  def contentType(json: MediaType): Response =
+    self.addHeader(Header.ContentType(json))


Rendering of the ContentType header is quite expensive. For this reason, at the bottom of the the Response companion object the main 3 content-types are cached after calling untyped on them (which invokes the render method).

Might be better to pattern match here and check if the media type here matches one of those 3 and use the cached value or fallback to Header.ContentType.apply if not?

Also the param name should be changed to mediaType

Maybe it is just the best to use the untyped API. Probably the cheapest.

kyri-petrou

Apologies, I missed some minor things in my last review. Otherwise LGTM

kyri-petrou · 2024-08-24T01:36:31Z

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

+  withErrorBody: Boolean = false,
+  withStackTrace: Boolean = false,
+  maxStackTraceDepth: Int = 10,
+  errorFormat: ErrorResponseConfig.ErrorFormat = ErrorResponseConfig.ErrorFormat.Html,


I'm thinking plaintext might make more sense as a default as it accounts for a wider range of usecases (APIs & serving user content)

kyri-petrou · 2024-08-24T01:38:09Z

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

+    Middleware.runBefore(configRef.updateSome { case oldConfig if oldConfig != config => config })
+
+  private[http] lazy val configRef: FiberRef[ErrorResponseConfig] =
+    FiberRef.unsafe.make(ErrorResponseConfig())(Unsafe)


For performance reasons, FiberRefs use reference equality to determine whether a FiberRef was changed. It's probably not an issue but perhaps use default here just incase

kyri-petrou · 2024-08-24T01:39:48Z

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

+        ErrorResponseConfig.configRef.get.map { cfg =>
+          if (cfg.withErrorBody) {
+            val mediaType: MediaType = accept
+              .flatMap(_.mimeTypes.sortBy(mt => -mt.qFactor.getOrElse(1.0)).map(_.mediaType).collectFirst {


Should we add an implicit Ordering in the companion object of Accept? 🤔

We could also ensure it is always sorted.

Best to ensure it's always sorted, we don't want to be doing this on each request.

It has to be done on each request, since this the the requests accept header. But since the amount of accepted media types will be small, I thought a sorted list might be easier to understand. It is just sorted by q factor when ever you deal with the typed interface.

Maybe you could check and avoid sorting it if it's already sorted, which would avoid the overhead entirely for the happy path.

kyri-petrou · 2024-08-24T01:50:14Z

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

@@ -58,6 +58,9 @@ final case class Response(
        self.copy(body = Body.fromChunk(bytes))
      }

+  def contentType(mediaType: MediaType): Response =
+    self.addHeader("content-type", mediaType.fullType)


Outside the scope of this PR, but I'm wondering whether we should change the fullType to a val instead of a lazy val to avoid the penalty of lazy vals.

Since both are static strings, I'll expect the val to be cheap anyway

If this would be scala 3 only, inline def is probably best?

Let's tackle this in another issue/PR where we can revise the usages of lazy val in ZIO HTTP. I think there are quite a few unnecessary ones

kyri-petrou · 2024-08-24T01:52:16Z

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

+        if (config.withStackTrace && throwable.getStackTrace.nonEmpty)
+          (if (config.maxStackTraceDepth == 0) throwable.getStackTrace
+           else throwable.getStackTrace.take(config.maxStackTraceDepth))
+            .mkString("\n", "\n", "")
+        else ""


Is getStackTrace cheap? Or should we rewrite this to invoke it only once?

I'll double check

I created a local val. I think we could optimize printing limited stack traces, since getStackTrace captures the full stack. We could probably walk the trace only until n. But I think this is okay for now.

kyri-petrou · 2024-08-24T01:54:02Z

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


  final def handleErrorZIO(f: Err => ZIO[Any, Nothing, Response])(implicit trace: Trace): Route[Env, Nothing] =
    self.handleErrorCauseZIO { cause =>
      cause.failureOrCause match {
        case Left(err)    => f(err)
-        case Right(cause) => ZIO.succeed(Response.fromCause(cause))
+        case Right(cause) => ErrorResponseConfig.configRef.get.map(Response.fromCause(cause, _))


Not as nice, but it requires 1 FlatMap less:

Suggested change

case Right(cause) => ErrorResponseConfig.configRef.get.map(Response.fromCause(cause, _))

case Right(cause) => ErrorResponseConfig.configRef.getWith(c => ZIO.succeed(Response.fromCause(cause, c)))

There are a few other places this can be applied to

Still nice enough

jdegoes · 2024-08-24T12:35:55Z

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

+ *   maximum number of stack trace lines to include in the response body. Set to
+ *   0 to include all lines.
+ */
+case class ErrorResponseConfig(


Suggested change

case class ErrorResponseConfig(

final case class ErrorResponseConfig(

jdegoes · 2024-08-24T12:42:15Z

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

@@ -549,7 +547,7 @@ sealed trait Handler[-R, +Err, -In, +Out] { self =>
    self(in)

  final def sandbox(implicit trace: Trace): Handler[R, Response, In, Out] =
-    self.mapErrorCause(Response.fromCause(_))
+    self.mapErrorCauseZIO(c => ErrorResponseConfig.configRef.get.map(Response.fromCause(c, _)).flip)


There's a fair amount of overhead here--of course, there was before, but now there is even more.

Could be useful to unsafely get the fiber ref value in as many places as you can "safely" bundle it up in lazy evaluation (like here).

I tried to figure out, what you mean. But I don't get it.
Currently, this config is saved within a fiber ref. Therefore idk how to get it unsafe. Also idk if the call to sandbox needs to be that optimized? Don't we advertise this as a non-prod feature?
But what you try to say might be true for other places?

One other design option I thought of was, to use a ConcurrentHashMap with the route pattern as a key. That way we could avoid fiber refs and zio based access to our config storage.

Ah right, FiberRef instead of Ref.

Wondering what the thought in using a fiber ref is? To make it compatible with scoped changes?

Yes. That you can override the behavior per route/request.

987Nabil requested review from jdegoes and vigoo as code owners August 23, 2024 07:41

algora-pbc bot mentioned this pull request Aug 23, 2024

Remove internal usage of warning header #2955

Closed

algora-pbc bot added the 🙋 Bounty claim label Aug 23, 2024

kyri-petrou reviewed Aug 23, 2024

View reviewed changes

kyri-petrou reviewed Aug 24, 2024

View reviewed changes

jdegoes reviewed Aug 24, 2024

View reviewed changes

987Nabil force-pushed the warning-headers branch 3 times, most recently from 3188189 to 8abbbee Compare August 27, 2024 20:26

jdegoes approved these changes Aug 27, 2024

View reviewed changes

987Nabil force-pushed the warning-headers branch 3 times, most recently from 1a0595c to 6f6bbd2 Compare August 27, 2024 21:37

987Nabil added 3 commits August 27, 2024 23:38

Remove warning header; Configurable generated error responses (zio#2955)

5d93e1b

Review changes

8830781

Review changes

ab9fdf7

987Nabil force-pushed the warning-headers branch from 6f6bbd2 to ab9fdf7 Compare August 27, 2024 21:40

987Nabil mentioned this pull request Aug 27, 2024

Remove warning header #2994

Closed

987Nabil merged commit 2c90dde into zio:main Aug 27, 2024
34 checks passed

987Nabil deleted the warning-headers branch August 27, 2024 22:10

This was referenced Aug 29, 2024

Why is warning header added by default? #2701

Closed

Warning header loses original exception when that exception contains invalid-for-netty-headers characters #2948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove warning header; Configurable generated error responses (#2955) #3050

Remove warning header; Configurable generated error responses (#2955) #3050

987Nabil commented Aug 23, 2024

kyri-petrou Aug 23, 2024

kyri-petrou Aug 23, 2024

kyri-petrou Aug 23, 2024

987Nabil Aug 23, 2024

kyri-petrou left a comment

kyri-petrou Aug 24, 2024

kyri-petrou Aug 24, 2024

kyri-petrou Aug 24, 2024

987Nabil Aug 24, 2024

jdegoes Aug 24, 2024

987Nabil Aug 24, 2024

jdegoes Aug 27, 2024

kyri-petrou Aug 24, 2024

987Nabil Aug 24, 2024

987Nabil Aug 24, 2024

kyri-petrou Aug 25, 2024 •

edited

Loading

kyri-petrou Aug 25, 2024

kyri-petrou Aug 24, 2024

987Nabil Aug 24, 2024

987Nabil Aug 25, 2024

kyri-petrou Aug 24, 2024

987Nabil Aug 24, 2024 •

edited

Loading

jdegoes Aug 24, 2024

jdegoes Aug 24, 2024

987Nabil Aug 25, 2024

jdegoes Aug 27, 2024

jdegoes Aug 27, 2024

987Nabil Aug 27, 2024

	case Right(cause) => ErrorResponseConfig.configRef.get.map(Response.fromCause(cause, _))
	case Right(cause) => ErrorResponseConfig.configRef.getWith(c => ZIO.succeed(Response.fromCause(cause, c)))

	case class ErrorResponseConfig(
	final case class ErrorResponseConfig(

Remove warning header; Configurable generated error responses (#2955) #3050

Remove warning header; Configurable generated error responses (#2955) #3050

Conversation

987Nabil commented Aug 23, 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

kyri-petrou left a 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

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

kyri-petrou Aug 25, 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

Choose a reason for hiding this comment

987Nabil Aug 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyri-petrou Aug 25, 2024 •

edited

Loading

987Nabil Aug 24, 2024 •

edited

Loading