-
Notifications
You must be signed in to change notification settings - Fork 61
feat: Support ProblemDetail #128
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package io.github.wimdeblauwe.errorhandlingspringbootstarter; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.http.ProblemDetail; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.net.URI; | ||
| import java.net.URISyntaxException; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| @Component | ||
| public class ProblemDetailFactory { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(ProblemDetailFactory.class); | ||
|
|
||
| private final ErrorHandlingProperties properties; | ||
|
|
||
| public ProblemDetailFactory(ErrorHandlingProperties properties) { | ||
| this.properties = properties; | ||
| } | ||
|
|
||
| public ProblemDetail build(ApiErrorResponse errorResponse) { | ||
| ProblemDetail problemDetail = ProblemDetail.forStatusAndDetail(Objects.requireNonNull(errorResponse.getHttpStatus()), errorResponse.getMessage()); | ||
| problemDetail.setDetail(errorResponse.getMessage()); | ||
| try { | ||
| problemDetail.setType(new URI(properties.getProblemDetailTypePrefix() + toKebabCase(errorResponse.getCode()))); | ||
| } catch (URISyntaxException ignored) { | ||
| } | ||
|
|
||
| HashMap<String, Object> allProperties = new HashMap<>(errorResponse.getProperties()); | ||
| List<ApiFieldError> fieldErrors = errorResponse.getFieldErrors(); | ||
| if (!fieldErrors.isEmpty()) { | ||
| allProperties.put("fieldErrors", fieldErrors); | ||
| } | ||
| List<ApiGlobalError> globalErrors = errorResponse.getGlobalErrors(); | ||
| if (!globalErrors.isEmpty()) { | ||
| allProperties.put("globalErrors", globalErrors); | ||
| } | ||
| problemDetail.setProperties(allProperties); | ||
|
|
||
| return problemDetail; | ||
| } | ||
|
|
||
| private String toKebabCase(String input) { | ||
| if (input.isEmpty()) { | ||
| return input; | ||
| } | ||
|
|
||
| String result = input | ||
| .replaceAll("([a-z])([A-Z])", "$1-$2") // camelCase boundaries | ||
| .replaceAll("([A-Z])([A-Z][a-z])", "$1-$2") // handle acronyms | ||
| .replaceAll("\\s+", "-") // spaces to hyphens | ||
| .replaceAll("_", "-") // underscores to hyphens | ||
| .toLowerCase(); | ||
| LOGGER.info("{} -> {}", input, result); | ||
| return result; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
|
|
||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.ApiErrorResponse; | ||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.ErrorHandlingFacade; | ||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.ErrorHandlingProperties; | ||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.ProblemDetailFactory; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.boot.autoconfigure.web.ErrorProperties; | ||
|
|
@@ -21,14 +23,21 @@ public class GlobalErrorWebExceptionHandler extends DefaultErrorWebExceptionHand | |
| private static final Logger LOGGER = LoggerFactory.getLogger(GlobalErrorWebExceptionHandler.class); | ||
|
|
||
| private final ErrorHandlingFacade errorHandlingFacade; | ||
| private final ErrorHandlingProperties errorHandlingProperties; | ||
| private final ProblemDetailFactory problemDetailFactory; | ||
|
|
||
|
|
||
| public GlobalErrorWebExceptionHandler(ErrorAttributes errorAttributes, | ||
| WebProperties.Resources resources, | ||
| ErrorProperties errorProperties, | ||
| ApplicationContext applicationContext, | ||
| ErrorHandlingFacade errorHandlingFacade) { | ||
| ErrorHandlingFacade errorHandlingFacade, | ||
| ErrorHandlingProperties errorHandlingProperties, | ||
| ProblemDetailFactory problemDetailFactory) { | ||
| super(errorAttributes, resources, errorProperties, applicationContext); | ||
| this.errorHandlingFacade = errorHandlingFacade; | ||
| this.errorHandlingProperties = errorHandlingProperties; | ||
| this.problemDetailFactory = problemDetailFactory; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -49,8 +58,14 @@ public Mono<ServerResponse> handleException(ServerRequest request) { | |
|
|
||
| ApiErrorResponse errorResponse = errorHandlingFacade.handle(Objects.requireNonNull(exception)); | ||
|
|
||
| return ServerResponse.status(Objects.requireNonNull(errorResponse.getHttpStatus())) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(BodyInserters.fromValue(errorResponse)); | ||
| if (errorHandlingProperties.isUseProblemDetailFormat()) { | ||
| return ServerResponse.status(Objects.requireNonNull(errorResponse.getHttpStatus())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the httpStatus in ApiErrorResponse be defined as non-null to avoid all the calls to Maybe there are some places (outside this PR), where
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, this is not possible. The Using |
||
| .contentType(MediaType.APPLICATION_PROBLEM_JSON) | ||
| .body(BodyInserters.fromValue(problemDetailFactory.build(errorResponse))); | ||
| } else { | ||
| return ServerResponse.status(Objects.requireNonNull(errorResponse.getHttpStatus())) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(BodyInserters.fromValue(errorResponse)); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,11 @@ | ||
| package io.github.wimdeblauwe.errorhandlingspringbootstarter.servlet; | ||
|
|
||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.ApiErrorResponse; | ||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.ErrorHandlingFacade; | ||
| import io.github.wimdeblauwe.errorhandlingspringbootstarter.*; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.http.ProblemDetail; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.ControllerAdvice; | ||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
|
|
@@ -21,9 +21,13 @@ public class ErrorHandlingControllerAdvice { | |
| private static final Logger LOGGER = LoggerFactory.getLogger(ErrorHandlingControllerAdvice.class); | ||
|
|
||
| private final ErrorHandlingFacade errorHandlingFacade; | ||
| private final ErrorHandlingProperties errorHandlingProperties; | ||
| private final ProblemDetailFactory problemDetailFactory; | ||
|
|
||
| public ErrorHandlingControllerAdvice(ErrorHandlingFacade errorHandlingFacade) { | ||
| public ErrorHandlingControllerAdvice(ErrorHandlingFacade errorHandlingFacade, ErrorHandlingProperties errorHandlingProperties, ProblemDetailFactory problemDetailFactory) { | ||
| this.errorHandlingFacade = errorHandlingFacade; | ||
| this.errorHandlingProperties = errorHandlingProperties; | ||
| this.problemDetailFactory = problemDetailFactory; | ||
| } | ||
|
|
||
| @ExceptionHandler | ||
|
|
@@ -33,8 +37,15 @@ public ResponseEntity<?> handleException(Throwable exception, WebRequest webRequ | |
|
|
||
| ApiErrorResponse errorResponse = errorHandlingFacade.handle(exception); | ||
|
|
||
| return ResponseEntity.status(Objects.requireNonNull(errorResponse.getHttpStatus())) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(errorResponse); | ||
| if (errorHandlingProperties.isUseProblemDetailFormat()) { | ||
| ProblemDetail problemDetail = problemDetailFactory.build(errorResponse); | ||
| return ResponseEntity.status(Objects.requireNonNull(errorResponse.getHttpStatus())) | ||
| .contentType(MediaType.APPLICATION_PROBLEM_JSON) | ||
| .body(problemDetail); | ||
| } else { | ||
| return ResponseEntity.status(Objects.requireNonNull(errorResponse.getHttpStatus())) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(errorResponse); | ||
| } | ||
|
Comment on lines
+40
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This if-else block is very similar to the if-else in 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
Inject the appropriate implementation of the interface into
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two sources of variation
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. |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the ProblemDetail spec requires the
typefield 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the examples I have found use kebab case for the type field. I guess because it is supposed to be a URL?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.