zio-http
zio-http copied to clipboard
Explicit and customizable error messages for Endpoint BadRequest (#2650)
@jdegoes I did not follow the plan as described in the ticket due to learnings while implementing it.
If the user does not handle the error explicitly, then we encode an error in a content-type acceptable to the HTTP client, perhaps defaulting to application/json. Moreover, we use a format that is somewhat standardized for returning error details. The main consideration, however, is just making sure that when users don't handle encoding/decoding errors, we do something that helps end-users diagnose and fix their encoding/decoding issues.
✅ 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.
We need an operator that lifts encoding/decoding errors (currently defects) into E, and we need to document that, show how it's used, and make it common practice to deal with it.
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 of Endpoint
seems wrong, because I think we should not make it possible to create a decoding error in the handler handed over to implement
. I instead chose, to have a new parameter of Endpoint
, that only defines how decoding errors are handled in terms of a Codec
. This way, we do not interfere with the Endpoint
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)
We need another operator that logs the codec errors in a standard way, useful for development.
Endpoint#logCodecError
does the job. But I am thinking if a more general tabCodecError
would be better.
The operators should be defined probably on Handler all the way up to Routes (including Route).
This does not seem to fit the general design.
fixes #2650 /claim #2650
The errors returned to the client are great! 👍
Clarification on .logCodecError
-
I thought that by adding that to my endpoint definition, zio-http would start logging the errors on the backend as well, but that doesn't seem to be happening.
Do I need to enable something else at the server level to start seeing these errors?
@swoogles I just tried it and .logCodecError(e => e.getMessage())
leads to a log message being printed with error level. Maybe your logging settings are the issue?
@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.
Codecov Report
Attention: Patch coverage is 49.52681%
with 160 lines
in your changes are missing coverage. Please review.
Project coverage is 64.48%. Comparing base (
2591f9f
) to head (24e5161
). Report is 2 commits behind head on main.
:exclamation: 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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@987Nabil Ping me when ready for a final review. Looking great!
@jdegoes Should be ready now
@987Nabil Looks like this needs latest changes in main
before it's ready.
@swoogles no. There are changes, but it is mergeable.