feat: Support ProblemDetail

[wimdeblauwe]

This PR adds support for ProblemDetail.

It is disabled by default and can be enabled via the error.handling.use-problem-detail-format=true property.

Mapping is done like this:

• type: Is the code, but converted to kebab case (e.g. validation-failed). Optionally, you can give the url a prefix via error.handling.problem-detail-type-prefix property (e.g. https://example.com/validation-failed).
• title: Is generated automatically by the Spring ProblemDetail class.
• detail: Is the message.

Additional properties (added via @ResponseErrorProperty) are also added to the result. For validation errors, the fieldErrors and globalErrors will also be present.

Fixes #59

[donalmurtagh]

title: Is generated automatically by the Spring ProblemDetail class.
detail: Is the message.

In the issue discussion, you proposed to map message to title, but it seems you changed your mind and have mapped message to detail instead.

For someone trying to migrate to the ProblemDetail support in v5.1.0 from an earlier version of the library, it would be helpful to understand how the title field is generated.

For example, if an instance of this exception is thrown

public class UserNotFoundException extends RuntimeException {
    public UserNotFoundException(UserId userId) {
        super("Could not find user with id " + userId);
    }
}

Previous to v5.1.0, with the default configuration this would return the following JSON with a 500 HTTP status

{
 "code": "USER_NOT_FOUND",
 "message": "Could not find user with id 123" 
}

In v5.1.0, with ProblemDetail support enabled this would return the following JSON with a 500 HTTP status

{
 "type": "user-not-found",
 "detail": "Could not find user with id 123",
 "title": "???" 
}

But what would the title field contain?

It would be helpful to see some examples of the title field for common error types such as

• server error
• validation failed
• not found (404)

[donalmurtagh]

Could the httpStatus in ApiErrorResponse be defined as non-null to avoid all the calls to Objects.requireNotNull?

Maybe there are some places (outside this PR), where getHttpStatus() is expected to return null that I've missed. If so, an alternative approach is to change getHttpStatus() to return Optional<HttpStatusCode> which would allow the @Nullable to be removed from the getter.

[wimdeblauwe]

Unfortunately, this is not possible.

The status field is not always present in the JSON output (it is mostly not present as this is the default). So when you want to serialize the JSON back to the object (using ApiErrorResponseDeserializer), then it can be null.

Using Optional would interfere with existing code. I am also not sure if it would influence the JSON serialization. This is something that could be considered for a next major version, but I don't want to do breaking changes at this point.

[donalmurtagh]

This if-else block is very similar to the if-else in GlobalErrorWebExceptionHandler.handleException.

Perhaps it could be improved (in terms of encapsulation/DRY) by defining a custom type for rendering the response, e.g.

public interface ResponseFactory {
  ServerResponse buildReactiveResponse(ApiErrorResponse error);

  ResponseEntity<?> buildServletResponse(ApiErrorResponse error);
}

and have 2 beans that implement this interface

1. DefaultResponseFactory - when ProblemDetail is disabled
2. ProblemDetailResponseFactory - when ProblemDetail is enabled

Inject the appropriate implementation of the interface into GlobalErrorWebExceptionHandler and ErrorHandlingControllerAdvice and use it instead of ProblemDetailFactory.

[wimdeblauwe]

Interesting idea, but what I don't like about it is that it mixes reactive and servlet classes in the same interface. i rather keep those parts separate.

[donalmurtagh]

There are two sources of variation

1. Reactive or servlet
2. ProblemDetail enabled or disabled

In the proposal above, (1) is handled via separate interface methods and (2) is handled via separate implementations of the interface

If you would prefer not to mix servlet and reactive code in the same type, perhaps it would be better to swap (1) and (2). In other words, define an interface with one method for generating the ProblemDetail enabled response, a second method for the ProblemDetail disabled response, and separate implementations of the interface for the reactive and servlet cases.

There are many other options for handling the various cases, but I do think that moving the response-generation to a dedicated type would be a cleaner design and eliminate some code duplication.

[donalmurtagh]

Unless the ProblemDetail spec requires the type field to be kebab case, It's not clear why you're changing the code to kebab case when ProblemDetail mode is enabled.

This will make it more difficult to migrate to ProblemDetail mode (e.g. changing frontend code), for no obvious benefit.

[wimdeblauwe]

All the examples I have found use kebab case for the type field. I guess because it is supposed to be a URL?

[donalmurtagh]

AFAIK, there's nothing that prohibits the use of uppercase or underscores in a URL, but obviously it's much more common to use lowercase and dashes. So you could argue that lowercase and dash-separators are a de facto standard.

Presumably a lot of applications using this starter will have switch-statements or if-else statements that check the type of the error, which will need to be migrated if ProblemDetail support is enabled. I can see 3 options

1. Stick with kebab case and mention (e.g. in the release notes) that the aforementioned migration will be necessary if ProblemDetail support is enabled
2. Use SNAKE_CASE for the error type in all cases
3. Use kebab case (1) by default, but provide a flag that allows SNAKE_CASE (2) to be used instead. Obviously, this is the option that provides the most flexibility for the user, but the most amount of effort for you

Personally, I think all 3 options are defensible, and even if someone does have to migrate, it's a pretty easy migration, so probably not worth worrying about too much.

[wimdeblauwe]

As a reply to #128 (comment):

I tested with the UserNotFoundException example and this is what I get:

{
  "detail": "Could not find user with id 1234567890",
  "instance": "/integration-test/not-found",
  "status": 500,
  "title": "Internal Server Error",
  "type": "user-not-found"
}

So title is really coming from the status code. If I annotated with @ResponseStatus(NOT_FOUND), then the result is:

{
  "detail": "Could not find user with id 1234567890",
  "instance": "/integration-test/not-found",
  "status": 404,
  "title": "Not Found",
  "type": "user-not-found"
}

For a validation error, status 400 is used and title is Bad Request. This is default behaviour that comes from ProblemDetail.forStatusAndDetail() from Spring Boot.

[donalmurtagh]

So title is really coming from the status code.

It would be helpful to mention this in the docs, and perhaps also include an example