zio-http icon indicating copy to clipboard operation
zio-http copied to clipboard

Explicit and customizable error messages for Endpoint BadRequest (#2650)

Open 987Nabil opened this issue 11 months ago • 5 comments

@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 tabCodecErrorwould 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

987Nabil avatar Mar 03 '24 11:03 987Nabil

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 avatar Mar 04 '24 18:03 swoogles

@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 avatar Mar 05 '24 07:03 987Nabil

@987Nabil Can you point me towards a working example? I've been tweaking settings without success.

swoogles avatar Mar 06 '24 21:03 swoogles

@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 avatar Mar 07 '24 07:03 987Nabil

@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.

swoogles avatar Mar 07 '24 07:03 swoogles

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.

Files Patch % Lines
...cala/zio/http/codec/internal/TextBinaryCodec.scala 20.68% 92 Missing :warning:
...ed/src/main/scala/zio/http/endpoint/Endpoint.scala 55.76% 23 Missing :warning:
...n/scala/zio/http/endpoint/openapi/OpenAPIGen.scala 60.86% 18 Missing :warning:
.../scala/zio/http/endpoint/HttpCodecErrorCodec.scala 55.88% 15 Missing :warning:
...c/main/scala/zio/http/codec/HttpContentCodec.scala 85.71% 5 Missing :warning:
...main/scala/zio/http/codec/internal/BodyCodec.scala 66.66% 4 Missing :warning:
...io-http/shared/src/main/scala/zio/http/Route.scala 0.00% 1 Missing :warning:
...o-http/shared/src/main/scala/zio/http/Routes.scala 0.00% 1 Missing :warning:
...scala/zio/http/codec/internal/EncoderDecoder.scala 90.90% 1 Missing :warning:

: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.

codecov-commenter avatar Apr 14 '24 10:04 codecov-commenter

@987Nabil Ping me when ready for a final review. Looking great!

jdegoes avatar Apr 16 '24 18:04 jdegoes

@jdegoes Should be ready now

987Nabil avatar Apr 19 '24 15:04 987Nabil

@987Nabil Looks like this needs latest changes in main before it's ready.

swoogles avatar Apr 19 '24 16:04 swoogles

@swoogles no. There are changes, but it is mergeable.

987Nabil avatar Apr 19 '24 16:04 987Nabil