Fix #7262: mulitpart decoder wrapping unwanted errors #7265
Conversation
| .handleError { | ||
| case e: InvalidMessageBodyFailure => Left(e) | ||
| case e => Left(InvalidMessageBodyFailure("Invalid multipart body", Some(e))) |
There was a problem hiding this comment.
Sorry for being late to the party. But I'd like to express my opinion without claiming to strictness. I could be wrong, but exceptions being thrown by public API become its contact. Here in MultipartDecoder, any exception is transformed to InvalidMessageBodyFailure. I guess we can't resolve the initial issue without impacting the public contract, but what if we try to minimise it? What I precisely suggest, in short:
- Move
org.http4s.server.middleware.EntityLimiter.EntityTooLargeto thecoremodule. - Use it in the
EntityLimiterMiddleware. - Propagate it directly in the error handling via
org.http4s.InvalidMessageBodyFailure#cause.
@armanbilge @LaurenceWarne wdyt?
There was a problem hiding this comment.
So we would maintain existing behaviour except for special casing the EntityTooLarge exception? If that would be preferable I'm happy to do it.
Propagate it directly in the error handling via org.http4s.InvalidMessageBodyFailure#cause.
I may not be understanding correctly, but InvalidMessageBodyFailure.message doesn't appear to use cause, so we wouldn't see the specific error (I guess we could change details)? An alternative could be to use Pull.raiseError[F](InvalidMessageBodyFailure("Entity too large")) in the middleware, though I guess this would alter existing behaviour also?
There was a problem hiding this comment.
but exceptions being thrown by public API become its contact
@danicheg I see what you're saying, but meh. The constraint is Concurrent[F] so users should be prepared for any Throwable. If the API was GenConcurrent[F, InvalidMessageBodyFailure] then I agree we could not change that explicit contract, but that's not the case. This was just a bug.
| .handleError { | ||
| case e: InvalidMessageBodyFailure => Left(e) | ||
| case e => Left(InvalidMessageBodyFailure("Invalid multipart body", Some(e))) | ||
| .recover { case e: DecodeFailure => |
There was a problem hiding this comment.
I think MessageBodyFailure is the right level of granularity. DecodeFailure is too general e.g. applies to headers.
| .recover { case e: DecodeFailure => | |
| .recover { case e: MessageBodyFailure => |
| mkDecoder.use { decoder => | ||
| val decoded = decoder.decode(request, true) | ||
| val result = decoded.value | ||
| assertIO(result.attempt, Left(CustomError)) | ||
| }.assert |
There was a problem hiding this comment.
| mkDecoder.use { decoder => | |
| val decoded = decoder.decode(request, true) | |
| val result = decoded.value | |
| assertIO(result.attempt, Left(CustomError)) | |
| }.assert | |
| mkDecoder.use(_.decode(request, true).value).intercept[CustomError.type] |
|
LGTM! |
|
Thanks! |
Hi, this PR implements a fix for #7262 by only catching errors related to decoding failures in
MultipartDecoder.Thanks!