jakartaee / rest

Jakarta RESTful Web Services
Other
361 stars 117 forks source link

Add ProblemDetails Support #1150

Open hantsy opened 1 year ago

hantsy commented 1 year ago

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

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

mkarg commented 1 year ago

Thank you for pointing us to this RFC!

My personal opinion in is:

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

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

hantsy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

chkal commented 1 year ago

Very interesting topic.

A few spontaneous thoughts:

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

mkarg commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

spericas commented 1 year ago

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 commented 1 year ago

@spericas So jaxrs no plan for it?

mkarg commented 1 year ago

@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 commented 6 months ago

@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 commented 6 months ago

@t1 Great work.