Skip to content
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

Enhancement: Support for RFC 7807 / 9457 #3199

Open
skewty opened this issue Mar 13, 2024 · 14 comments · May be fixed by #3323
Open

Enhancement: Support for RFC 7807 / 9457 #3199

skewty opened this issue Mar 13, 2024 · 14 comments · May be fixed by #3323
Labels
Enhancement This is a new feature or request Help Wanted 🆘 This is good for people to work on

Comments

@skewty
Copy link

skewty commented Mar 13, 2024

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.
Fund with Polar
@skewty skewty added the Enhancement This is a new feature or request label Mar 13, 2024
@JacobCoffee
Copy link
Member

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.

@skewty skewty changed the title Enhancement: Support for RFC 7807 Enhancement: Support for RFC 7807 / 9457 Mar 15, 2024
@skewty
Copy link
Author

skewty commented Mar 15, 2024

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?

@JacobCoffee JacobCoffee added the Help Wanted 🆘 This is good for people to work on label Mar 15, 2024
@kedod
Copy link
Contributor

kedod commented Mar 15, 2024

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
Copy link
Member

guacs commented Mar 16, 2024

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
Copy link
Member

guacs commented Mar 16, 2024

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
Copy link

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:

  1. Providing a new exception type, e.g. ProblemDetailsException, that can be raised, resulting in a problem details response.
  2. 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
Copy link
Member

guacs commented Mar 20, 2024

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:

  1. Providing a new exception type, e.g. ProblemDetailsException, that can be raised, resulting in a problem details response.
  2. 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
Copy link
Member

guacs commented Mar 20, 2024

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
Copy link

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
Copy link

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
Copy link
Author

skewty commented Mar 21, 2024

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
Copy link
Member

guacs commented Mar 22, 2024

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
Copy link

bunny-therapist commented Mar 22, 2024

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
Copy link
Member

guacs commented Mar 26, 2024

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

@peterschutt peterschutt linked a pull request Apr 5, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request Help Wanted 🆘 This is good for people to work on
Projects
Status: Ideas
Development

Successfully merging a pull request may close this issue.

5 participants