Refactor setting of problem detail type (#4099)

#### What type of PR is this? /kind improvement /area core #### What this PR does / why we need it: Define a global map to mapping exception to problem detail type. #### Does this PR introduce a user-facing change? ```release-note None ```
halo-dev · Jun 21, 2023 · 5e9e875 · 5e9e875
1 parent 12a426c
commit 5e9e875
Show file tree

Hide file tree

Showing 6 changed files with 77 additions and 79 deletions.
diff --git a/application/src/main/java/run/halo/app/infra/exception/Exceptions.java b/application/src/main/java/run/halo/app/infra/exception/Exceptions.java
@@ -1,8 +1,29 @@
 package run.halo.app.infra.exception;
 
+import static org.springframework.core.annotation.MergedAnnotations.SearchStrategy.TYPE_HIERARCHY;
+
+import io.github.resilience4j.ratelimiter.RequestNotPermitted;
+import java.net.URI;
+import java.time.Instant;
+import java.util.Locale;
+import java.util.Map;
+import lombok.extern.slf4j.Slf4j;
+import org.springframework.context.MessageSource;
+import org.springframework.core.annotation.MergedAnnotations;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.HttpStatusCode;
+import org.springframework.lang.Nullable;
+import org.springframework.security.authentication.BadCredentialsException;
+import org.springframework.web.ErrorResponse;
+import org.springframework.web.bind.annotation.ResponseStatus;
+import org.springframework.web.server.ServerWebExchange;
+
+@Slf4j
 public enum Exceptions {
     ;
 
+    public static final String DEFAULT_TYPE = "about:blank";
+
     public static final String THEME_ALREADY_EXISTS_TYPE =
         "https://halo.run/probs/theme-alreay-exists";
 
@@ -12,4 +33,46 @@ public enum Exceptions {
     public static final String REQUEST_NOT_PERMITTED_TYPE =
         "https://halo.run/probs/request-not-permitted";
 
+    /**
+     * Non-ErrorResponse exception to type map.
+     */
+    public static final Map<Class<? extends Throwable>, String> EXCEPTION_TYPE_MAP = Map.of(
+        RequestNotPermitted.class, REQUEST_NOT_PERMITTED_TYPE,
+        BadCredentialsException.class, INVALID_CREDENTIAL_TYPE
+    );
+
+    public static ErrorResponse createErrorResponse(Throwable t, @Nullable HttpStatusCode status,
+        ServerWebExchange exchange, MessageSource messageSource) {
+        final ErrorResponse errorResponse;
+        if (t instanceof ErrorResponse er) {
+            errorResponse = er;
+        } else {
+            var responseStatusAnno =
+                MergedAnnotations.from(t.getClass(), TYPE_HIERARCHY).get(ResponseStatus.class);
+            if (status == null) {
+                status = responseStatusAnno.getValue("code", HttpStatus.class)
+                    .orElse(HttpStatus.INTERNAL_SERVER_ERROR);
+            }
+            var type = EXCEPTION_TYPE_MAP.getOrDefault(t.getClass(), DEFAULT_TYPE);
+            var detail = responseStatusAnno.getValue("reason", String.class)
+                .orElseGet(t::getMessage);
+            var builder = ErrorResponse.builder(t, status, detail)
+                .type(URI.create(type));
+            if (status.is5xxServerError()) {
+                builder.detailMessageCode("problemDetail.internalServerError")
+                    .titleMessageCode("problemDetail.title.internalServerError");
+            }
+            errorResponse = builder.build();
+        }
+        var problemDetail = errorResponse.updateAndGetBody(messageSource, getLocale(exchange));
+        problemDetail.setInstance(exchange.getRequest().getURI());
+        problemDetail.setProperty("requestId", exchange.getRequest().getId());
+        problemDetail.setProperty("timestamp", Instant.now());
+        return errorResponse;
+    }
+
+    public static Locale getLocale(ServerWebExchange exchange) {
+        var locale = exchange.getLocaleContext().getLocale();
+        return locale == null ? Locale.getDefault() : locale;
+    }
 }
diff --git a/...ion/src/main/java/run/halo/app/infra/exception/handlers/ProblemDetailErrorAttributes.java b/...ion/src/main/java/run/halo/app/infra/exception/handlers/ProblemDetailErrorAttributes.java
@@ -1,23 +1,13 @@
 package run.halo.app.infra.exception.handlers;
 
-import static org.springframework.core.annotation.MergedAnnotations.SearchStrategy;
-import static org.springframework.core.annotation.MergedAnnotations.from;
+import static run.halo.app.infra.exception.Exceptions.createErrorResponse;
 
-import java.net.URI;
-import java.time.Instant;
 import java.util.LinkedHashMap;
-import java.util.Locale;
 import java.util.Map;
-import java.util.Optional;
 import org.springframework.boot.web.error.ErrorAttributeOptions;
 import org.springframework.boot.web.reactive.error.DefaultErrorAttributes;
 import org.springframework.boot.web.reactive.error.ErrorAttributes;
 import org.springframework.context.MessageSource;
-import org.springframework.core.annotation.MergedAnnotation;
-import org.springframework.http.HttpStatus;
-import org.springframework.http.HttpStatusCode;
-import org.springframework.web.ErrorResponse;
-import org.springframework.web.bind.annotation.ResponseStatus;
 import org.springframework.web.reactive.function.server.ServerRequest;
 import org.springframework.web.server.ServerWebExchange;
 
@@ -41,50 +31,12 @@ public ProblemDetailErrorAttributes(MessageSource messageSource) {
     public Map<String, Object> getErrorAttributes(ServerRequest request,
         ErrorAttributeOptions options) {
         final var errAttributes = new LinkedHashMap<String, Object>();
-
         var error = getError(request);
-        var responseStatusAnno = from(error.getClass(), SearchStrategy.TYPE_HIERARCHY)
-            .get(ResponseStatus.class);
-
-        var status = determineHttpStatus(error, responseStatusAnno);
-        final ErrorResponse errorResponse;
-        if (error instanceof ErrorResponse er) {
-            errorResponse = er;
-        } else {
-            var reason = Optional.of(status)
-                .filter(HttpStatusCode::is5xxServerError)
-                .map(s -> "Something went wrong, please try again later.")
-                .orElseGet(() -> responseStatusAnno.getValue("reason", String.class)
-                    .orElse(error.getMessage())
-                );
-            errorResponse = ErrorResponse.create(error, status, reason);
-        }
-        var problemDetail =
-            errorResponse.updateAndGetBody(messageSource, getLocale(request.exchange()));
-
-        problemDetail.setInstance(URI.create(request.path()));
-        problemDetail.setProperty("requestId", request.exchange().getRequest().getId());
-        problemDetail.setProperty("timestamp", Instant.now());
-
-        // For backward compatibility(rendering view need)
-        errAttributes.put("error", problemDetail);
+        var errorResponse = createErrorResponse(error, null, request.exchange(), messageSource);
+        errAttributes.put("error", errorResponse.getBody());
         return errAttributes;
     }
 
-    private HttpStatusCode determineHttpStatus(Throwable t,
-        MergedAnnotation<ResponseStatus> responseStatusAnno) {
-        if (t instanceof ErrorResponse rse) {
-            return rse.getStatusCode();
-        }
-        return responseStatusAnno.getValue("code", HttpStatus.class)
-            .orElse(HttpStatus.INTERNAL_SERVER_ERROR);
-    }
-
-    private Locale getLocale(ServerWebExchange exchange) {
-        var locale = exchange.getLocaleContext().getLocale();
-        return locale != null ? locale : Locale.getDefault();
-    }
-
     @Override
     public Throwable getError(ServerRequest request) {
         return (Throwable) request.attribute(ERROR_INTERNAL_ATTRIBUTE).stream()

diff --git a/...c/main/java/run/halo/app/security/authentication/login/UsernamePasswordAuthenticator.java b/...c/main/java/run/halo/app/security/authentication/login/UsernamePasswordAuthenticator.java
@@ -3,21 +3,16 @@
 import static org.springframework.http.HttpStatus.TOO_MANY_REQUESTS;
 import static org.springframework.http.HttpStatus.UNAUTHORIZED;
 import static org.springframework.http.MediaType.APPLICATION_JSON;
-import static run.halo.app.infra.exception.Exceptions.INVALID_CREDENTIAL_TYPE;
-import static run.halo.app.infra.exception.Exceptions.REQUEST_NOT_PERMITTED_TYPE;
+import static run.halo.app.infra.exception.Exceptions.createErrorResponse;
 import static run.halo.app.security.authentication.WebExchangeMatchers.ignoringMediaTypeAll;
 
 import io.github.resilience4j.ratelimiter.RateLimiterRegistry;
 import io.github.resilience4j.ratelimiter.RequestNotPermitted;
 import io.github.resilience4j.reactor.ratelimiter.operator.RateLimiterOperator;
 import io.micrometer.observation.ObservationRegistry;
-import java.net.URI;
-import java.time.Instant;
-import java.util.Locale;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.context.MessageSource;
 import org.springframework.http.HttpMethod;
-import org.springframework.http.HttpStatus;
 import org.springframework.lang.NonNull;
 import org.springframework.security.authentication.ObservationReactiveAuthenticationManager;
 import org.springframework.security.authentication.ReactiveAuthenticationManager;
@@ -143,30 +138,16 @@ private <T> RateLimiterOperator<T> createIPBasedRateLimiter(ServerWebExchange ex
 
     private Mono<Void> handleRequestNotPermitted(RequestNotPermitted e,
         ServerWebExchange exchange) {
-        var errorResponse =
-            createErrorResponse(e, TOO_MANY_REQUESTS, REQUEST_NOT_PERMITTED_TYPE, exchange);
+        var errorResponse = createErrorResponse(e, TOO_MANY_REQUESTS, exchange, messageSource);
         return writeErrorResponse(errorResponse, exchange);
     }
 
     private Mono<Void> handleAuthenticationException(AuthenticationException exception,
         ServerWebExchange exchange) {
-        var errorResponse =
-            createErrorResponse(exception, UNAUTHORIZED, INVALID_CREDENTIAL_TYPE, exchange);
+        var errorResponse = createErrorResponse(exception, UNAUTHORIZED, exchange, messageSource);
         return writeErrorResponse(errorResponse, exchange);
     }
 
-    private ErrorResponse createErrorResponse(Throwable t, HttpStatus status, String type,
-        ServerWebExchange exchange) {
-        var errorResponse =
-            ErrorResponse.create(t, status, t.getMessage());
-        var problemDetail = errorResponse.updateAndGetBody(messageSource, getLocale(exchange));
-        problemDetail.setType(URI.create(type));
-        problemDetail.setInstance(exchange.getRequest().getURI());
-        problemDetail.setProperty("requestId", exchange.getRequest().getId());
-        problemDetail.setProperty("timestamp", Instant.now());
-        return errorResponse;
-    }
-
     private Mono<Void> writeErrorResponse(ErrorResponse errorResponse,
         ServerWebExchange exchange) {
         return ServerResponse.status(errorResponse.getStatusCode())
@@ -175,11 +156,6 @@ private Mono<Void> writeErrorResponse(ErrorResponse errorResponse,
             .flatMap(response -> response.writeTo(exchange, context));
     }
 
-    private Locale getLocale(ServerWebExchange exchange) {
-        var locale = exchange.getLocaleContext().getLocale();
-        return locale == null ? Locale.getDefault() : locale;
-    }
-
     private class UsernamePasswordAuthenticationWebFilter extends AuthenticationWebFilter {
 
         public UsernamePasswordAuthenticationWebFilter(

diff --git a/application/src/main/resources/config/i18n/messages.properties b/application/src/main/resources/config/i18n/messages.properties
@@ -19,6 +19,7 @@ problemDetail.title.run.halo.app.infra.exception.PluginInstallationException=Plu
 problemDetail.title.run.halo.app.infra.exception.PluginAlreadyExistsException=Plugin Already Exists Error
 problemDetail.title.run.halo.app.infra.exception.DuplicateNameException=Duplicate Name Error
 problemDetail.title.io.github.resilience4j.ratelimiter.RequestNotPermitted=Request Not Permitted
+problemDetail.title.internalServerError=Internal Server Error
 
 # Detail definitions
 problemDetail.org.springframework.web.server.UnsupportedMediaTypeStatusException=Content type {0} is not supported. Supported media types: {1}.
@@ -50,3 +51,4 @@ problemDetail.directoryTraversal=Directory traversal detected. Base path is {0},
 
 problemDetail.plugin.version.unsatisfied.requires=Plugin requires a minimum system version of {0}, but the current version is {1}.
 problemDetail.plugin.missingManifest=Missing plugin manifest file "plugin.yaml" or manifest file does not conform to the specification.
+problemDetail.internalServerError=Something went wrong, please try again later.
diff --git a/application/src/main/resources/config/i18n/messages_zh.properties b/application/src/main/resources/config/i18n/messages_zh.properties
@@ -7,6 +7,7 @@ problemDetail.title.run.halo.app.infra.exception.DuplicateNameException=名称
 problemDetail.title.run.halo.app.infra.exception.PluginAlreadyExistsException=插件已存在
 problemDetail.title.run.halo.app.infra.exception.ThemeInstallationException=主题安装失败
 problemDetail.title.io.github.resilience4j.ratelimiter.RequestNotPermitted=请求限制
+problemDetail.title.internalServerError=服务器内部错误
 
 problemDetail.org.springframework.security.authentication.BadCredentialsException=用户名或密码错误。
 problemDetail.run.halo.app.infra.exception.AttachmentAlreadyExistsException=文件 {0} 已存在，建议更名后重试。
@@ -21,4 +22,5 @@ problemDetail.plugin.version.unsatisfied.requires=插件要求一个最小的系
 problemDetail.plugin.missingManifest=缺少 plugin.yaml 配置文件或配置文件不符合规范。
 
 problemDetail.theme.version.unsatisfied.requires=主题要求一个最小的系统版本为 {0}, 但当前版本为 {1}。
-problemDetail.theme.install.missingManifest=缺少 theme.yaml 配置文件或配置文件不符合规范。
+problemDetail.theme.install.missingManifest=缺少 theme.yaml 配置文件或配置文件不符合规范。
+problemDetail.internalServerError=服务器内部发生错误，请稍候再试。
diff --git a/application/src/test/resources/config/i18n/messages.properties b/application/src/test/resources/config/i18n/messages.properties
@@ -1,3 +1,6 @@
 problemDetail.title.run.halo.app.infra.exception.handlers.I18nExceptionTest$ErrorResponseException=Error Response
+problemDetail.internalServerError=Something went wrong, please try again later.
+
 problemDetail.run.halo.app.infra.exception.handlers.I18nExceptionTest$ErrorResponseException=Message argument is {0}.
 error.somethingWentWrong=Something went wrong, argument is {0}.
+problemDetail.title.internalServerError=Internal Server Error