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

Add ProblemDetails Support #1150

Open
hantsy opened this issue May 25, 2023 · 19 comments
Open

Add ProblemDetails Support #1150

hantsy opened this issue May 25, 2023 · 19 comments

Comments

@hantsy
Copy link

hantsy commented May 25, 2023

Spring has already added ProblemDetails support in the new Spring 6.

https://www.rfc-editor.org/rfc/rfc7807

@mkarg
Copy link
Contributor

mkarg commented May 27, 2023

Thank you for pointing us to this RFC!

My personal opinion in is:

  • This RFC looks very useful for application programmers, as it standardizes a channel to transport error, cause, and details from server to client, so an application can show more information to the end user without the need to re-invent a custom solution per application.
  • Spring 6's ProblemDetail does not seem to be any simpler to use than simply writing a custom record and return it from within an ExceptionMapper.
  • I doubt that much can be done by the Jakarta REST specification to support RFC 7807 better than it is already. JSON and XML are perfectly easy to be done in 3.1 already. Setting up record consisting of the mandatory fields by the author of an exeception mapper is more than simple already, while custom fields need any kind of custom code anyways (even if the custom code simply means a lengthy chain of builder method).
  • Automation (like any kind of "magic" to set up the information in a declarative way, e. g. more annotations and providers) seem to make the solution unnecessarily complex and provides no additional gain. OTOH what I would that Jakarta REST is such magic. What I envision is that ExceptionMapper automatically produces a RFC 7807 result by inspecting the exception itself. For example, I think an optional annotation or interface at the exception could provide the needed details.

Having said that, I like to present some counter-proposal to discuss (not counter proposals to RFC 7807, but to ProblemDetail like in Spring 6):

  • Interface: We could provide an interface that any exception class can implement, and which provides details to the ExceptionMapper on the cause, type, description, and so on. This is mostly straight-forward old-school plain Java, so everybody would rather quickly be familiar with using it, and adoption would happen quickly.
  • Annotation: We could provide an annotation that any exception class can hold, and that provides static information on the details (like mapping the type URI on the exception class).
  • Provider: We could provide a new class of providers which tell the exception mapper where to find the needed information.

My personal favorite is the interface solution, as providing the cause in the same way we already provide the error feals like Java.

@hantsy
Copy link
Author

hantsy commented May 27, 2023

Spring 6's ProblemDetail does not seem to be any simpler to use than simply writing a custom record and return it from within an ExceptionMapper.

In Spring 6, when handling a response body of ProblemDetails, it will update the HTTP status(code and message) with status value in the problemDetails object automatically if the response is not set HTTP Status explicitly.

@mkarg
Copy link
Contributor

mkarg commented May 27, 2023

Spring 6's ProblemDetail does not seem to be any simpler to use than simply writing a custom record and return it from within an ExceptionMapper.

In Spring 6, when handling a response body of ProblemDetails, it will update the HTTP status(code and message) with status value in the problemDetails object automatically if the response is not set HTTP Status explicitly.

Understood, but as Jakarta REST's way of error handling is an ExceptionMapper (not a resource method result), that exception handler must gain the details somehow. So you spare just one single line in the exception mapper (setting the status code), but still you need to write the code to set up the ProblemDetails instance. Does not feel like lots being spared. Is it worth that?

If we go with MyException implements DetailProvider this seemlessly integrates with existing applications and (depending on the actual implementation in the exception class) can pull much more information automatically from the exception - infos that one must push into ProblemDetails with custom code otherwise.

@hantsy
Copy link
Author

hantsy commented May 27, 2023

I hope there is an annotation which can handle a collection of exceptions like the Spring @ExceptionHandler(can be placed on the method in controller or a @ControllerAdvice compoent), and return a ProblemDetails, Response with ProblemDetails body.

For me, ExceptionMapper handles exceptions one by one is a little tedious.

@mkarg
Copy link
Contributor

mkarg commented May 27, 2023

I am confused by your last posting. Is it a beginner's question or a feature proposal?

@chkal
Copy link
Contributor

chkal commented May 29, 2023

Very interesting topic.

A few spontaneous thoughts:

  • Currently, the behavior of the default exception mapper is not defined in detail, which leaves it up to the implementations. It would be great if exceptions, for which there is no custom exception mapper, would result in an RFC-7807 response if the content type is JSON or XML out of the box.
  • In this context, it may be helpful to provide a record that defines the RFC-7807 structure so that even custom exception mappers could emit RFC-7807 without too much boilerplate.
  • But I also agree with @mkarg that having some interface or annotation-based approach for creating the problem details from exceptions directly would be really great. This would allow enhancing existing application exceptions by implementing an interface or adding annotation so that ideally all the problem details fields can be populated without building custom exception mappers. Although, it should still be possible to create the problem details programmatically in case of custom exception mappers.

Just my 2 cents. Would love to hear more thoughts.

@mkarg
Copy link
Contributor

mkarg commented May 29, 2023

Reading @chkal's response I think it would be a wise and extensible solution if we do not discuss ProblemDescription versus interface / annotation, but instead discuss how to provide both. Jakarta REST could come with a class (not a record, as a record is not custom-extensible) and the rule that any compliant implement MUST process it according RFC-7807 on one hand (including the fact that such a class provides the status code), but on the other hand provide a rule that a compliant implementation MUST have a default exception mapper that produces ProblemDescription by inspecting the actual exception (using an interface or annotation).

@spericas
Copy link
Contributor

spericas commented Jun 1, 2023

My initial reaction to this is that, although an interesting RFC for sure, it probably shouldn't be part of the core Jakarta REST (or in any way integrated with default exception mappers). Given our API's extensibility, it should be possible for implementations to provide an additional module to support this RFC is they so choose. This alternative would make it easy for application developers to opt in without the need to possibly need to opt out.

@mkarg
Copy link
Contributor

mkarg commented Jun 2, 2023

@spericas Recently you were +1 for integrating status codes that also could easily get added using the extension mechanism, so I am rather confused about your comment. Can you elaborate why you are +1 for the status codes but -1 for this RFC?

@spericas
Copy link
Contributor

spericas commented Jun 5, 2023

@mkarg Unless I'm missing something, adding a few more status codes is very different from incorporating an RFC that defines a machine-readable payload format that Jakarta REST would need to follow.

@mkarg
Copy link
Contributor

mkarg commented Jun 11, 2023

@mkarg Unless I'm missing something, adding a few more status codes is very different from incorporating an RFC that defines a machine-readable payload format that Jakarta REST would need to follow.

Actually I do not see such a difference, a feature is a feature to me, independent of its size or complexity (in fact, those new status codes do not bring much benefit, but this new payload would, IMHO). But I certainly respect your opinion. So is your -1 really a veto (i. e. we MUST NOT adopt this RFC, even in case a majority wants to)? Could I convince you to change it from a vetoing -1 to a 0?

@spericas
Copy link
Contributor

@mkarg Unless I'm missing something, adding a few more status codes is very different from incorporating an RFC that defines a machine-readable payload format that Jakarta REST would need to follow.

Actually I do not see such a difference, a feature is a feature to me, independent of its size or complexity (in fact, those new status codes do not bring much benefit, but this new payload would, IMHO). But I certainly respect your opinion. So is your -1 really a veto (i. e. we MUST NOT adopt this RFC, even in case a majority wants to)? Could I convince you to change it from a vetoing -1 to a 0?

You're usually the first one to be in opposition to adding new functionality to the "core JAX-RS" (you've used that argument many times in the past). It is exactly the same thing I'm saying here, I don't see why this couldn't be part of an extension in an implementation.

So, no, not all features are or should be consider equally (and certainly supporting an RFC such as this and adding a few more error codes is not the same). I could change my vote after hearing the right argument as to why this should be part of the core.

@mkarg
Copy link
Contributor

mkarg commented Jun 12, 2023

To be more precise: I am not generally in opposition to any new functionality, I am just in opposition to new functionality that can easily be done in an external library. ;-)

Don't get me wrong, I have no problem with doing this features in an external library-- I really love the idea to provide some kind of "JAX-RS Essentials" extension lib. I just was confused because I expected that you will be +1 here, just because you were +1 for the status codes. I am absolutely fine with doing this in an external library, if you really want to put a veto here.

@jamezp
Copy link
Contributor

jamezp commented Aug 9, 2023

Just for reference, it looks like #839 was requesting this as well and it was closed.

@spericas
Copy link
Contributor

Just for reference, it looks like #839 was requesting this as well and it was closed.

I would be in favor of closing this one too :)

@hantsy
Copy link
Author

hantsy commented Aug 24, 2023

@spericas So jaxrs no plan for it?

@mkarg
Copy link
Contributor

mkarg commented Aug 24, 2023

@hantsy Santiago already pointed out that he is vetoing unless we provide a good reason. The discussion as shown that there is no such good reason. Hence I am closing this issue hereby. The feature can easily get implemented by a third-party library thanks to the various extension points of JAX-RS (like filters, interceptors, features, services, etc.). The proposed feature undoubtly might be commonly beneficial to a lot of applications, so I like to encourage you to start an open source project for providing such a filter, and put a link to it here.

@t1
Copy link

t1 commented Mar 12, 2024

@mkarg : you wanted to close this issue 😁

BTW: four years ago, I started to write such a library and a blog about this, if anybody happens to be still interested. I even considered the client side, so a problem detail response can be mapped back to a business exception.

@hantsy
Copy link
Author

hantsy commented Mar 20, 2024

@t1 Great work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants