From f75748c55cd360bb41b52a086f8221f2460f6be4 Mon Sep 17 00:00:00 2001 From: asr2003 <162500856+asr2003@users.noreply.github.com> Date: Sun, 10 Nov 2024 03:08:45 +0530 Subject: [PATCH 1/3] Implement codec error handling --- .../scala/zio/http/ErrorResponseConfig.scala | 3 +- .../zio/http/codec/HttpContentCodec.scala | 53 +++++++++++-- .../zio/http/codec/internal/BodyCodec.scala | 77 ++++++++++++------- 3 files changed, 96 insertions(+), 37 deletions(-) diff --git a/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala b/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala index 10237403a8..7f8e39f55c 100644 --- a/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala +++ b/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala @@ -22,6 +22,7 @@ final case class ErrorResponseConfig( withStackTrace: Boolean = false, maxStackTraceDepth: Int = 10, errorFormat: ErrorResponseConfig.ErrorFormat = ErrorResponseConfig.ErrorFormat.Html, + logCodecErrors: Boolean = false, ) object ErrorResponseConfig { @@ -34,7 +35,7 @@ object ErrorResponseConfig { val default: ErrorResponseConfig = ErrorResponseConfig() val debugConfig: ErrorResponseConfig = - ErrorResponseConfig(withErrorBody = true, withStackTrace = true, maxStackTraceDepth = 0) + ErrorResponseConfig(withErrorBody = true, withStackTrace = true, maxStackTraceDepth = 0, logCodecErrors = true) private[http] val configRef: FiberRef[ErrorResponseConfig] = FiberRef.unsafe.make(default)(Unsafe) diff --git a/zio-http/shared/src/main/scala/zio/http/codec/HttpContentCodec.scala b/zio-http/shared/src/main/scala/zio/http/codec/HttpContentCodec.scala index 3f8a3f29a2..30ca37c7ba 100644 --- a/zio-http/shared/src/main/scala/zio/http/codec/HttpContentCodec.scala +++ b/zio-http/shared/src/main/scala/zio/http/codec/HttpContentCodec.scala @@ -32,10 +32,23 @@ sealed trait HttpContentCodec[A] { self => lookup(contentType) match { case Some((_, codec)) => request.body.asChunk.flatMap { bytes => - ZIO.fromEither(codec.codec(config).decode(bytes)) + ZIO + .fromEither(codec.codec(config).decode(bytes)) + .mapError(_ => CodecDecodeError(s"Failed to decode request body for media type: $contentType")) + .tapError(error => + ErrorResponseConfig.configRef.get.flatMap { errConfig => + if (errConfig.logCodecErrors) ZIO.logWarning(error.getMessage) else ZIO.unit + }, + ) } case None => - ZIO.fail(throw new IllegalArgumentException(s"No codec found for content type $contentType")) + ZIO + .fail(UnsupportedMediaTypeError(contentType)) + .tapError(error => + ErrorResponseConfig.configRef.get.flatMap { errConfig => + if (errConfig.logCodecErrors) ZIO.logWarning(error.getMessage) else ZIO.unit + }, + ) } } @@ -47,10 +60,23 @@ sealed trait HttpContentCodec[A] { self => lookup(contentType) match { case Some((_, codec)) => response.body.asChunk.flatMap { bytes => - ZIO.fromEither(codec.codec(config).decode(bytes)) + ZIO + .fromEither(codec.codec(config).decode(bytes)) + .mapError(_ => CodecDecodeError(s"Failed to decode response body for media type: $contentType")) + .tapError(error => + ErrorResponseConfig.configRef.get.flatMap { errConfig => + if (errConfig.logCodecErrors) ZIO.logWarning(error.getMessage) else ZIO.unit + }, + ) } case None => - ZIO.fail(throw new IllegalArgumentException(s"No codec found for content type $contentType")) + ZIO + .fail(UnsupportedMediaTypeError(contentType)) + .tapError(error => + ErrorResponseConfig.configRef.get.flatMap { errConfig => + if (errConfig.logCodecErrors) ZIO.logWarning(error.getMessage) else ZIO.unit + }, + ) } } @@ -112,9 +138,9 @@ sealed trait HttpContentCodec[A] { self => private[http] def chooseFirstOrDefault( mediaTypes: Chunk[MediaTypeWithQFactor], - ): (MediaType, BinaryCodecWithSchema[A]) = + ): Either[UnsupportedMediaTypeError, (MediaType, BinaryCodecWithSchema[A])] = if (mediaTypes.isEmpty) { - (defaultMediaType, defaultBinaryCodecWithSchema) + Right((defaultMediaType, defaultBinaryCodecWithSchema)) } else { var i = 0 var result: (MediaType, BinaryCodecWithSchema[A]) = null @@ -124,8 +150,10 @@ sealed trait HttpContentCodec[A] { self => if (lookupResult.isDefined) result = lookupResult.get i += 1 } - if (result == null) (defaultMediaType, defaultBinaryCodecWithSchema) - else result + if (result == null) { + ZIO.logWarning(s"Unsupported media type: ${mediaTypes.head.mediaType}") + Right((defaultMediaType, defaultBinaryCodecWithSchema)) + } else Right(result) } def lookup(mediaType: MediaType): Option[(MediaType, BinaryCodecWithSchema[A])] @@ -180,6 +208,15 @@ sealed trait HttpContentCodec[A] { self => } +sealed trait CodecError extends Throwable +case class UnsupportedMediaTypeError(mediaType: MediaType) extends CodecError { + override def getMessage: String = s"Unsupported media type: $mediaType" +} + +case class CodecDecodeError(details: String) extends CodecError { + override def getMessage: String = s"Codec decode error: $details" +} + object HttpContentCodec { final case class Choices[A]( choices: ListMap[MediaType, BinaryCodecWithSchema[A]], diff --git a/zio-http/shared/src/main/scala/zio/http/codec/internal/BodyCodec.scala b/zio-http/shared/src/main/scala/zio/http/codec/internal/BodyCodec.scala index feb9f64ee1..fa77a098b5 100644 --- a/zio-http/shared/src/main/scala/zio/http/codec/internal/BodyCodec.scala +++ b/zio-http/shared/src/main/scala/zio/http/codec/internal/BodyCodec.scala @@ -109,9 +109,11 @@ private[http] object BodyCodec { final case class Single[A](codec: HttpContentCodec[A], name: Option[String]) extends BodyCodec[A] { - def mediaType(accepted: Chunk[MediaTypeWithQFactor]): Option[MediaType] = - Some(codec.chooseFirstOrDefault(accepted)._1) - + def mediaType(accepted: Chunk[MediaTypeWithQFactor]): Option[MediaType] = + codec.chooseFirstOrDefault(accepted) match { + case Right((mediaType, _)) => Some(mediaType) + case Left(_) => None + } def decodeFromField(field: FormField, config: CodecConfig)(implicit trace: Trace): IO[Throwable, A] = { val codec0 = codec .lookup(field.contentType) @@ -139,27 +141,36 @@ private[http] object BodyCodec { def encodeToField(value: A, mediaTypes: Chunk[MediaTypeWithQFactor], name: String, config: CodecConfig)(implicit trace: Trace, ): FormField = { - val (mediaType, bc @ BinaryCodecWithSchema(_, _)) = codec.chooseFirstOrDefault(mediaTypes) - if (mediaType.binary) { - FormField.binaryField( - name, - bc.codec(config).encode(value), - mediaType, - ) - } else { - FormField.textField( - name, - bc.codec(config).encode(value).asString, - mediaType, - ) + codec.chooseFirstOrDefault(mediaTypes) match { + + case Right((mediaType, bc @ BinaryCodecWithSchema(_, _))) => + if (mediaType.binary) { + FormField.binaryField( + name, + bc.codec(config).encode(value), + mediaType, + ) + } else { + FormField.textField( + name, + bc.codec(config).encode(value).asString, + mediaType, + ) + } + case Left(error) => + throw new IllegalArgumentException(s"Unsupported media type: ${error.mediaType}") } } def encodeToBody(value: A, mediaTypes: Chunk[MediaTypeWithQFactor], config: CodecConfig)(implicit trace: Trace, ): Body = { - val (mediaType, bc @ BinaryCodecWithSchema(_, _)) = codec.chooseFirstOrDefault(mediaTypes) - Body.fromChunk(bc.codec(config).encode(value), mediaType) + codec.chooseFirstOrDefault(mediaTypes) match { + case Right((mediaType, bc @ BinaryCodecWithSchema(_, _))) => + Body.fromChunk(bc.codec(config).encode(value), mediaType) + case Left(error) => + throw new IllegalArgumentException(s"Unsupported media type: ${error.mediaType}") + } } type Element = A @@ -169,8 +180,10 @@ private[http] object BodyCodec { extends BodyCodec[ZStream[Any, Nothing, E]] { def mediaType(accepted: Chunk[MediaTypeWithQFactor]): Option[MediaType] = - Some(codec.chooseFirstOrDefault(accepted)._1) - + codec.chooseFirstOrDefault(accepted) match { + case Right((mediaType, _)) => Some(mediaType) + case Left(_) => None + } override def decodeFromField(field: FormField, config: CodecConfig)(implicit trace: Trace, ): IO[Throwable, ZStream[Any, Nothing, E]] = @@ -200,12 +213,16 @@ private[http] object BodyCodec { )(implicit trace: Trace, ): FormField = { - val (mediaType, bc) = codec.chooseFirstOrDefault(mediaTypes) - FormField.streamingBinaryField( - name, - value >>> bc.codec(config).streamEncoder, - mediaType, - ) + codec.chooseFirstOrDefault(mediaTypes) match { + case Right((mediaType, bc)) => + FormField.streamingBinaryField( + name, + value >>> bc.codec(config).streamEncoder, + mediaType, + ) + case Left(error) => + throw new IllegalArgumentException(s"Unsupported media type: ${error.mediaType}") + } } override def encodeToBody( @@ -215,8 +232,12 @@ private[http] object BodyCodec { )(implicit trace: Trace, ): Body = { - val (mediaType, bc @ BinaryCodecWithSchema(_, _)) = codec.chooseFirstOrDefault(mediaTypes) - Body.fromStreamChunked(value >>> bc.codec(config).streamEncoder).contentType(mediaType) + codec.chooseFirstOrDefault(mediaTypes) match { + case Right((mediaType, bc @ BinaryCodecWithSchema(_, _))) => + Body.fromStreamChunked(value >>> bc.codec(config).streamEncoder).contentType(mediaType) + case Left(error) => + throw new IllegalArgumentException(s"Unsupported media type: ${error.mediaType}") + } } type Element = E From 9824fdfec0276ae42929811cd1f52685824076e1 Mon Sep 17 00:00:00 2001 From: asr2003 <162500856+asr2003@users.noreply.github.com> Date: Mon, 11 Nov 2024 10:04:41 +0530 Subject: [PATCH 2/3] Make Mima happier --- .../main/scala/zio/http/ErrorResponseConfig.scala | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala b/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala index 7f8e39f55c..6fc75779e4 100644 --- a/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala +++ b/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala @@ -33,7 +33,17 @@ object ErrorResponseConfig { case object Json extends ErrorFormat { val mediaType: MediaType = MediaType.application.json } } - val default: ErrorResponseConfig = ErrorResponseConfig() + // Backward-compatible apply method for older usage, without logCodecErrors + def apply( + withErrorBody: Boolean, + withStackTrace: Boolean, + maxStackTraceDepth: Int, + errorFormat: ErrorFormat, + ): ErrorResponseConfig = + new ErrorResponseConfig(withErrorBody, withStackTrace, maxStackTraceDepth, errorFormat, logCodecErrors = false) + + val default: ErrorResponseConfig = ErrorResponseConfig() + val debugConfig: ErrorResponseConfig = ErrorResponseConfig(withErrorBody = true, withStackTrace = true, maxStackTraceDepth = 0, logCodecErrors = true) From b050abacc37c4f62244194d1b4d51cd280ec8740 Mon Sep 17 00:00:00 2001 From: asr2003 <162500856+asr2003@users.noreply.github.com> Date: Mon, 11 Nov 2024 12:43:23 +0530 Subject: [PATCH 3/3] alter approach to make mima happier --- .../scala/zio/http/ErrorResponseConfig.scala | 35 ++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala b/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala index 6fc75779e4..a82750315c 100644 --- a/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala +++ b/zio-http/shared/src/main/scala/zio/http/ErrorResponseConfig.scala @@ -22,12 +22,39 @@ final case class ErrorResponseConfig( withStackTrace: Boolean = false, maxStackTraceDepth: Int = 10, errorFormat: ErrorResponseConfig.ErrorFormat = ErrorResponseConfig.ErrorFormat.Html, - logCodecErrors: Boolean = false, -) + logCodecErrors: Boolean = false +) { + + /** + * Backward-compatible copy method for compatibility with older code. + * + * Omits the new `logCodecErrors` parameter, which defaults to `false` in + * older usage scenarios. + */ + def copy( + withErrorBody: Boolean = this.withErrorBody, + withStackTrace: Boolean = this.withStackTrace, + maxStackTraceDepth: Int = this.maxStackTraceDepth, + errorFormat: ErrorResponseConfig.ErrorFormat = this.errorFormat + ): ErrorResponseConfig = + new ErrorResponseConfig(withErrorBody, withStackTrace, maxStackTraceDepth, errorFormat, logCodecErrors) + + /** + * Full copy method including all parameters. + */ + def copyWithLog( + withErrorBody: Boolean = this.withErrorBody, + withStackTrace: Boolean = this.withStackTrace, + maxStackTraceDepth: Int = this.maxStackTraceDepth, + errorFormat: ErrorResponseConfig.ErrorFormat = this.errorFormat, + logCodecErrors: Boolean = this.logCodecErrors + ): ErrorResponseConfig = + new ErrorResponseConfig(withErrorBody, withStackTrace, maxStackTraceDepth, errorFormat, logCodecErrors) +} object ErrorResponseConfig { sealed trait ErrorFormat { val mediaType: MediaType } - object ErrorFormat { + object ErrorFormat { case object Text extends ErrorFormat { val mediaType: MediaType = MediaType.text.`plain` } case object Html extends ErrorFormat { val mediaType: MediaType = MediaType.text.html } case object Json extends ErrorFormat { val mediaType: MediaType = MediaType.application.json } @@ -38,7 +65,7 @@ object ErrorResponseConfig { withErrorBody: Boolean, withStackTrace: Boolean, maxStackTraceDepth: Int, - errorFormat: ErrorFormat, + errorFormat: ErrorFormat ): ErrorResponseConfig = new ErrorResponseConfig(withErrorBody, withStackTrace, maxStackTraceDepth, errorFormat, logCodecErrors = false)