-
Notifications
You must be signed in to change notification settings - Fork 386
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
Explicit and customizable error messages for Endpoint BadRequest (#2650) #2714
Conversation
d20cd21
to
dc4e291
Compare
The errors returned to the client are great! 👍 Clarification on |
@swoogles I just tried it and |
@987Nabil Can you point me towards a working example? I've been tweaking settings without success. |
@swoogles btw this does not work for mismatches in the path. It will always lead to 404 without any log or custom error response. val getUser =
Endpoint(Method.GET / "users" / int("userId")).in[Int].out[Int].logCodecError(e => e.getMessage())
val getUserRoute =
getUser.implement {
handler { (id: Int, _: Int) =>
id
}
} |
@987Nabil Yep, that's what I would expect. In my case I am giving bad payloads to correct URLs and hoping to see the failure details for 400s. |
@@ -181,6 +180,18 @@ object HttpContentCodec { | |||
} | |||
} | |||
|
|||
object html { | |||
implicit val htmlCodec: HttpContentCodec[Dom] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicits should not have to be imported. This means this should be located in the companion object of Dom
or in the companion object of HttpContentCodec
.
import zio.http.{MediaType, Response} | ||
|
||
final case class CodecErrorHandler[A]( | ||
log: Option[HttpCodecError => String], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a super weird structure. Usually when I make something with this type of structure, I try to step back and throw away irrelevant details or rethink the abstraction.
First off, I would inline & delete MapError
, as without it, this class doesn't carry its weight.
Then you have:
log: Option[HttpCodecError => String],
f: (HttpCodecError, Chunk[MediaTypeWithQFactor]) => Err,
codec: HttpCodec[HttpCodecType.ResponseType, Err],
This is still super weird. Picking it apart by capabilities:
- It MIGHT have the capability to turn an
HttpCodecError
into aString
(for purposes of logging?). - It can turn an
HttpCodecError
into anErr
. - It can turn an
Err
into response, and a response into anErr
.
A few observations:
- It seems like we are going from
HttpCodecError
toErr
only so that we can then go into a response. - It is odd to pass
Chunk[MediaTypeWithQFactor]
into the functionf
. Why? Because it must return anErr
, which is a user-defined error type that, presumably, has no protocol-specific details, such as media type. The only case where this is used seems to be a type-unsafeErr
(wildcard type used,_
), which further supports the hypothesis that the media type should not be relevant forErr
.
Now, let's consider the use case:
The user wants to encode
HttpCodecError
in a way that is consistent with how they are encoding their own errors, so they can provide a uniform API experience to developers working with the API.
Note that this use case, if correct, is one-sided: there is no requirement for a ZIO HTTP Endpoint client of the API to be able to decode some codec error into a typed error (in fact, it's not really recoverable at that stage).
That means we can probably get away with:
decodeError: (HttpCodecError, Request) => Response,
domainError: HttpCodec[HttpCodecType.ResponseType, Err],
(This allows you to go to string if you want, for logging purposes, inasmuch as Response can be string-ified intelligently.)
On the other hand, if we need the codec errors in the typed error channel, then we might try:
decodeError: HttpCodec[HttpCodecType.ResponseType, HttpCodecError],
domainError: HttpCodec[HttpCodecType.ResponseType, Err],
This one says we can go both ways: from codec errors into responses, and back again; and from typed errors into responses, and back again. The user has control over how to turn HttpCodecError
into a response, without "contaminating" their own E
(typed error channel).
This encoding also gets ride of the media types because you can use fallback, perhaps, for different media types.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at this, I see the second formulation does not allow the automatic HTTP client to handle Err
in a type-safe way. I am not sure if that's a problem because codec errors should not happen.
/** | ||
* Hides any details of codec errors from the user. | ||
*/ | ||
def codecErrorEmptyResponse: Endpoint[PathInput, Input, Err, Output, Middleware] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emptyErrorResponse
254aaea
to
e8c3e73
Compare
6b37180
to
416d69d
Compare
@@ -13,25 +13,31 @@ import zio.http.Header.Accept.MediaTypeWithQFactor | |||
import zio.http._ | |||
import zio.http.internal.HeaderOps | |||
|
|||
final case class BinaryCodecWithSchema[A](codec: BinaryCodec[A], schema: Schema[A]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in BinaryCodecWithSchema.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
416d69d
to
24e5161
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2714 +/- ##
==========================================
+ Coverage 64.41% 64.48% +0.06%
==========================================
Files 148 150 +2
Lines 8669 8779 +110
Branches 1545 1566 +21
==========================================
+ Hits 5584 5661 +77
- Misses 3085 3118 +33 ☔ View full report in Codecov by Sentry. |
@@ -29,10 +29,10 @@ final case class MediaType( | |||
) { | |||
lazy val fullType: String = s"$mainType/$subType" | |||
|
|||
def matches(other: MediaType): Boolean = | |||
def matches(other: MediaType, ignoreParameters: Boolean = false): Boolean = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a 2nd matches, rather than introducing a boolean parameter with a default. Maybe matchesIgnoreParams
.
zio-http/shared/src/main/scala/zio/http/codec/BinaryCodecWithSchema.scala
Outdated
Show resolved
Hide resolved
@@ -334,7 +334,7 @@ object HttpCodec extends ContentCodecs with HeaderCodecs with MethodCodecs with | |||
def unused: HttpCodec[Any, ZNothing] = Halt | |||
|
|||
final case class Enumeration[Value](unit: Unit) extends AnyVal { | |||
def apply[AtomTypes, Sub1 <: Value: ClassTag, Sub2 <: Value: ClassTag]( | |||
def f2[AtomTypes, Sub1 <: Value: ClassTag, Sub2 <: Value: ClassTag]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these all renamed??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changing the order, the compiler could not infer the right apply. So I gave them explicit names.
|
||
case cause => Handler.failCause(cause) | ||
Handler.fromFunctionZIO { (request: zio.http.Request) => | ||
val error = cause.defects.head.asInstanceOf[HttpCodecError] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't know this is an HttpCodecError. It could be other defects as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do know. That is what line 202 isHttpCodecError
is checking :) That was even there before my changes. All others go to the default case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be best to do it in one step then, rather than check first and extract later (Option extraction, for example). In any case, if the logic is correct due to the branch its in (I didn't see that), my mistake!
doc, | ||
mw, | ||
) | ||
|
||
// /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code
Endpoint( | ||
route, | ||
input, | ||
output = self.output | (HttpCodec.content[Output2](mediaType) ++ StatusCodec.Ok ?? doc), | ||
output = (HttpCodec.content[Output2](mediaType) ++ StatusCodec.Ok ?? doc) | self.output, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
), | ||
) | ||
|
||
val default: HttpCodec[HttpCodecType.ResponseType, HttpCodecError] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has type HttpCodec
, so it should be inside the companion object of HttpCodec
. Could be called responseErrorCodec
or something. That would also eliminate this useless object that only has a single static field.
@987Nabil Ping me when ready for a final review. Looking great! |
…chema.scala Co-authored-by: John A. De Goes <john@degoes.net>
d6cc95c
to
318681f
Compare
318681f
to
1fedb49
Compare
@jdegoes Should be ready now |
@987Nabil Looks like this needs latest changes in |
@swoogles no. There are changes, but it is mergeable. |
@jdegoes I did not follow the plan as described in the ticket due to learnings while implementing it.
✅ The solution is checking the accept header and falling back to json
❌ I did not follow any of these error formats. Why? Because there are so many and I have no good reason to follow any. I chose the simplest possible solution. If devs want to follow a format, they should imho implement it themselves.
In the solution on main, the decoder errors are handled in
Endpoint#implement
. And I think this makes sense. But this means, that after we have routes/handler, we can't lift the error type anymore, since it is handled. Lifting it on the level ofEndpoint
seems wrong, because I think we should not make it possible to create a decoding error in the handler handed over toimplement
. I instead chose, to have a new parameter ofEndpoint
, that only defines how decoding errors are handled in terms of aCodec
. This way, we do not interfere with theEndpoint
error type, but still have the possibility to analyse the possible errors and add them for example to open api (not impl. in this PR)Endpoint#logCodecError
does the job. But I am thinking if a more generaltabCodecError
would be better.This does not seem to fit the general design.
fixes #2650
/claim #2650