Enhancement: Support for RFC 7807 / 9457

skewty commented 8 months ago

Summary

A Boolean option to enable RFC7807 support might be a good way to enable this otherwise breaking change.

ie: app = Litestar(rfc7807=True)

Perhaps some of the common exceptions (like ValueError) could have:

type URI enpoint / page provided by Litestar (plugin?)
title field set automatically
status field set automatically (use same value determination as current)
detail field set automatically

A new Exception class perhaps JsonProblem(HTTPException) which auto-includes "application/problem+json" in header and has fields for: type, title, status, etc.

This would be nice to have and complement OpenAPI / Swagger support with the emerging standard for error responses.

Basic Example

No response

Drawbacks and Impact

No response

Unresolved questions

No response

[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

If you would like to see an issue prioritized, make a pledge towards it!

We receive the pledge once the issue is completed & verified

This, along with engagement in the community, helps us know which features are a priority to our users.

JacobCoffee commented 8 months ago

Discord Message

> guacs — Yesterday at 9:00 PM Thanks for this! I think this shouldn't be difficult to implement. It's just the matter of deciding how the API would look like. We could create a plugin which would inject an exception handler that would then convert all the errors into the response following the spec. Or maybe take in a config value rfc9457 that will determine the default exception handler. I like the second approach of taking in a config value, because then users would be able to control it at all the layers so if they only want these problem details for a single controller or route, they could do that. NOTE: I only skimmed through the RFC, but 7807 has been obsoleted by [9457](https://datatracker.ietf.org/doc/html/rfc9457).

skewty commented 8 months ago

NOTE: I only skimmed through the RFC, but 7807 has been obsoleted by 9457.

Oops, I had both open as tabs in browser and used the wrong one when I wrote the issue up. RFC 9457 should obviously be the target goal.

take in a config value rfc9457 that will determine the default exception handler

@guacs Are you thinking something like a mapping?

RFC9457Handler: TypeAlias = Callable[[Request, Exception], RFC9457Response]

def my_rfc9457_handler(request: Request, exception: Exception) -> RFC9457Response: ...

rfc9457_config: dict[type[Controller], RFC9457Handler] = {
    # any controller missing from the list would not have RFC9457
    # error support -- the default handler would be used instead
    MyController: my_rfc9457_handler,
}

Maybe the community could help produce a "default" RFC9457 handler for pydantic's ValidationError and some common ValueError, TypeError, LookupError, etc.

It then becomes trivial to handle some exceptions as custom and return whatever the default handler would produce for others.

What I haven't figured out or put much thought into yet is the URIs being referenced. Should a default RFC9457Controller be created that can be used for the "type" field of the response? It would work in conjunction with the default handler mentioned above.

To me, if the default handler and controller were to be created a plugin in another git repository would be best. Would the plugin just be middleware at that point?

kedod commented 8 months ago

Plugin will be plugin :) https://docs.litestar.dev/latest/usage/plugins.html

Part of the code can look like this:


def my_rfc9457_handler(request: Request, exception: Exception) -> RFC9457Response: ...

class RFC9457Plugin(InitPluginProtocol):
    def on_app_init(self, app_config: AppConfig) -> AppConfig:
        app_config.exception_handlers[HTTPException] = my_rfc9457_handler
        return app_config

app = Litestar(plugins=[RFC9457Plugin()])

guacs commented 8 months ago

Plugin will be plugin :) https://docs.litestar.dev/latest/usage/plugins.html

Part of the code can look like this:

def my_rfc9457_handler(request: Request, exception: Exception) -> RFC9457Response: ...

class RFC9457Plugin(InitPluginProtocol):
    def on_app_init(self, app_config: AppConfig) -> AppConfig:
        app_config.exception_handlers[HTTPException] = my_rfc9457_handler
        return app_config

app = Litestar(plugins=[RFC9457Plugin()])

This is exactly what I had in mind.

Though there are some questions like how to set up type URL that's part of the spec. My immediate thought is that the plugin takes in a default value and uses that unless the caught exception is a new JsonProblemError (a better name will be needed) in which case it would have all the required fields and would then use that.

Another question is whether this plugin should be included in the main repo or whether this should be implemented as a separate library. Considering that this is an official RFC and an effort towards standardization, I'm kind of leaning towards including it the main litestar repo.

guacs commented 8 months ago

Maybe the community could help produce a "default" RFC9457 handler for pydantic's ValidationError and some common ValueError, TypeError, LookupError, etc.

I like this idea. Though within litestar, I think we should only catch any litestar specific exceptions rather than ValueError etc.

bunny-therapist commented 7 months ago

I would suggest ProblemDetailsException and ProblemDetailsPlugin since the RFC has already been updated once and the intention seems to be to support the latest version of it (?). It is also a much simpler and more descriptive name than "RFC 9457".

I recently added support to Litestar for the "+json" media type suffix partially because I wanted to make use of problem details (so now "application/problem+json" gets json encoded automatically - I think problem details may even be used as an example in the docs).

I would expect this feature to work by:

Providing a new exception type, e.g. ProblemDetailsException, that can be raised, resulting in a problem details response.
Optionally (if so configured thru some flag to the plugin) converting HTTPException to ProblemDetails responses instead of the usual litestar error response. This would also apply to e.g. validation errors and similar built in things. This way ProblemDetails becomes the default type of error responses from the application. The ProblemDetailsException can be used to provide more direct control of the problem details fields than whatever the conversion from HTTPException to ProblemDetails would be doing.

I think it is totally reasonable to convert the usual HTTPException to a problem details response because the problem details fields are optional, and there is some overlap (I think both have a "detail" field for example).

guacs commented 7 months ago

I would suggest ProblemDetailsException and ProblemDetailsPlugin since the RFC has already been updated once and the intention seems to be to support the latest version of it (?). It is also a much simpler and more descriptive name than "RFC 9457".

I recently added support to Litestar for the "+json" media type suffix partially because I wanted to make use of problem details (so now "application/problem+json" gets json encoded automatically - I think problem details may even be used as an example in the docs).

I would expect this feature to work by:

Providing a new exception type, e.g. ProblemDetailsException, that can be raised, resulting in a problem details response.

Optionally (if so configured thru some flag to the plugin) converting HTTPException to ProblemDetails responses instead of the usual litestar error response. This would also apply to e.g. validation errors and similar built in things. This way ProblemDetails becomes the default type of error responses from the application. The ProblemDetailsException can be used to provide more direct control of the problem details fields than whatever the conversion from HTTPException to ProblemDetails would be doing.

I think it is totally reasonable to convert the usual HTTPException to a problem details response because the problem details fields are optional, and there is some overlap (I think both have a "detail" field for example).

I agree with all your points. I agree that the names you suggested are better since it makes it easier to understand than specifying some RFC which could at some point be superceded by another RFC.

guacs commented 7 months ago

I read through the RFC and here are some initial thoughts/doubts I had regarding the implementation.

Should we convert all the HTTPExceptions into problem type definitions? Quoting the spec:

"Likewise, truly generic problems -- i.e., conditions that might apply to any resource on the Web -- are usually better expressed as plain status codes. For example, a "write access disallowed" problem is probably unnecessary, since a 403 Forbidden status code in response to a PUT request is self-explanatory."

Or we could just consider all of them as about:blank problem type as defined in section 4.2.1.
How can we do a generic conversion from ValidationError into a problem type since the problem type would vary based on the resource the client is trying to access? Maybe we can use the opt dictionary to get different details we'll need for the problem detail? This would allow for customizing the problem detail per route.
Should we add some kind of support for building the absolute URI for type (using route_reverse)?
Maintain support for the problem types defined in HTTP problem types registry? If we are doing this, I think we can do this at a later point once the basic APIs for problem types are created. To be fair, I don't know exactly what "maintain support for problem types defined in the HTTP problem types registry" means, but it could be specialized exceptions for those specific problem types.
We should also add support for the OpenAPI schema generation. Should this be generated for all routes? Or simply the routes that may return a problem type reponse? If the latter, then how would we be able to figure out which routes may return a problem type response? Refer Appendix A for the schema we should be generating.

bunny-therapist commented 7 months ago

I haven't had time to read the full RFC, so I can't address all of that right now. I assumed that whenever litestar returns an error code with some json data, that could be replaced by the plugin with problem details (ValidationError give something with detail(s) if memory serves me right) and any fields that we can't figure out can be omitted. I would have to look into the stuff about the problem type and the resource being accessed.

As for the comment about OpenAPI schema generation - I considered suggesting that, because I know that this fastapi plugin does that. However, I felt that this was a separate issue. Litestar currently does not add every possible error response for each endpoint into the Open API schema, so I am not sure it makes sense that error responses should be put there just because of their media type.

I would consider "add possible error responses to Open API schema" a separate issue from this. However, there may be something I am missing, of course.

bunny-therapist commented 7 months ago

Ok, read it now. I think it would be fine if to either use "about:blank" as type, or to even go so far as to define some predefined types in litestar for e.g. validation errors and such things that are already handled inside litestar itself.

skewty commented 7 months ago

Ok, read it now. I think it would be fine if to either use "about:blank" as type, or to even go so far as to define some predefined types in litestar for e.g. validation errors and such things that are already handled inside litestar itself.

This was my vision as well. Especially since we can, for example, use pydantic models as endpoint function parameter types for POST which can automatically generate 400 series status codes before even going into the function where the user can raise the appropriate exception type.

guacs commented 7 months ago

Ok, read it now. I think it would be fine if to either use "about:blank" as type, or to even go so far as to define some predefined types in litestar for e.g. validation errors and such things that are already handled inside litestar itself.

How would the predefined types work? From what I understood, they should be (or it's preferred) for them to be resolvable URIs that's specific to your application. Or have I misunderstood the spec?

bunny-therapist commented 7 months ago

Ok, read it now. I think it would be fine if to either use "about:blank" as type, or to even go so far as to define some predefined types in litestar for e.g. validation errors and such things that are already handled inside litestar itself.

How would the predefined types work? From what I understood, they should be (or it's preferred) for them to be resolvable URIs that's specific to your application. Or have I misunderstood the spec?

For example something like [app netloc]/problems/validation for ValidationError raised by litestar.

This is an example from the RFC in the same style:

 {
    "type": "https://example.com/probs/out-of-credit",
    "title": "You do not have enough credit.",
    "detail": "Your current balance is 30, but that costs 50.",
    "instance": "/account/12345/msgs/abc",
    "balance": 30,
    "accounts": ["/account/12345",
                 "/account/67890"]
   }

guacs commented 7 months ago

@bunny-therapist, @skewty I have a draft PR up with a very minimal implementation. If you have any feedback, that'd be great!

github-actions[bot] commented 2 months ago

This issue has been closed in #3323. The change will be included in the upcoming patch release.

github-actions[bot] commented 2 months ago

A fix for this issue has been released in v2.11.0

litestar-org / litestar