Respond 409 for operation conflict instead of 500 (#6274)

#### What type of PR is this?

/kind improvement
/area core
/kind api-change
/milestone 2.18.x

#### What this PR does / why we need it:

This PR makes ConcurrencyFailureException respond http status code 409 instead of 500.

#### Which issue(s) this PR fixes:

Fixes #6254 

#### Does this PR introduce a user-facing change?

```release-note
None
```
pull/6278/head
John Niang 2024-07-05 17:02:38 +08:00 committed by GitHub
parent 138d52e731
commit 708b8be792
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 73 additions and 17 deletions

View File

@ -10,8 +10,11 @@ import java.util.Map;
import lombok.extern.slf4j.Slf4j;
import org.springframework.context.MessageSource;
import org.springframework.core.annotation.MergedAnnotations;
import org.springframework.dao.ConcurrencyFailureException;
import org.springframework.http.HttpStatus;
import org.springframework.http.HttpStatusCode;
import org.springframework.http.ProblemDetail;
import org.springframework.lang.NonNull;
import org.springframework.lang.Nullable;
import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.web.ErrorResponse;
@ -33,6 +36,9 @@ public enum Exceptions {
public static final String REQUEST_NOT_PERMITTED_TYPE =
"https://halo.run/probs/request-not-permitted";
public static final String CONFLICT_TYPE =
"https://halo.run/probs/conflict";
/**
* Non-ErrorResponse exception to type map.
*/
@ -47,22 +53,11 @@ public enum Exceptions {
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 er = handleConflictException(t);
if (er == null) {
er = handleException(t, status);
}
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();
errorResponse = er;
}
var problemDetail = errorResponse.updateAndGetBody(messageSource, getLocale(exchange));
problemDetail.setInstance(exchange.getRequest().getURI());
@ -71,6 +66,39 @@ public enum Exceptions {
return errorResponse;
}
@NonNull
private static ErrorResponse handleException(Throwable t, @Nullable HttpStatusCode status) {
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");
}
return builder.build();
}
@Nullable
private static ErrorResponse handleConflictException(Throwable t) {
if (t instanceof ConcurrencyFailureException) {
return ErrorResponse.builder(t, ProblemDetail.forStatus(HttpStatus.CONFLICT))
.type(URI.create(CONFLICT_TYPE))
.titleMessageCode("problemDetail.title.conflict")
.detailMessageCode("problemDetail.conflict")
.build();
}
return null;
}
public static Locale getLocale(ServerWebExchange exchange) {
var locale = exchange.getLocaleContext().getLocale();
return locale == null ? Locale.getDefault() : locale;

View File

@ -28,6 +28,7 @@ problemDetail.title.run.halo.app.infra.exception.PluginDependencyException$Wrong
problemDetail.title.run.halo.app.infra.exception.PluginDependentsNotDisabledException=Dependents Not Disabled
problemDetail.title.run.halo.app.infra.exception.PluginDependenciesNotEnabledException=Dependencies Not Enabled
problemDetail.title.internalServerError=Internal Server Error
problemDetail.title.conflict=Conflict
# Detail definitions
problemDetail.org.springframework.web.server.UnsupportedMediaTypeStatusException=Content type {0} is not supported. Supported media types: {1}.
@ -72,6 +73,7 @@ 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.
problemDetail.conflict=Conflict detected, please check the data and retry.
problemDetail.migration.backup.notFound=The backup file does not exist or has been deleted.
title.visibility.identification.private=(Private)

View File

@ -16,6 +16,7 @@ problemDetail.title.run.halo.app.infra.exception.PluginDependencyException$Wrong
problemDetail.title.run.halo.app.infra.exception.PluginDependentsNotDisabledException=子插件未禁用
problemDetail.title.run.halo.app.infra.exception.PluginDependenciesNotEnabledException=依赖未启用
problemDetail.title.internalServerError=服务器内部错误
problemDetail.title.conflict=冲突
problemDetail.org.springframework.security.authentication.BadCredentialsException=用户名或密码错误。
problemDetail.run.halo.app.infra.exception.AttachmentAlreadyExistsException=文件 {0} 已存在,建议更名后重试。
@ -44,6 +45,7 @@ problemDetail.theme.version.unsatisfied.requires=主题要求一个最小的系
problemDetail.theme.install.missingManifest=缺少 theme.yaml 配置文件或配置文件不符合规范。
problemDetail.theme.install.alreadyExists=主题 {0} 已存在。
problemDetail.internalServerError=服务器内部发生错误,请稍候再试。
problemDetail.conflict=检测到冲突,请检查数据后重试。
problemDetail.migration.backup.notFound=备份文件不存在或已删除。
title.visibility.identification.private=(私有)

View File

@ -10,12 +10,14 @@ import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.dao.ConcurrencyFailureException;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.ProblemDetail;
import org.springframework.http.ResponseEntity;
import org.springframework.test.web.reactive.server.WebTestClient;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.bind.annotation.RestController;
@ -117,6 +119,21 @@ class I18nExceptionTest {
});
}
@Test
void shouldGetConflictError() {
webClient.put().uri("/response-entity/conflict-error")
.header("X-XSRF-TOKEN", "fake-token")
.cookie("XSRF-TOKEN", "fake-token")
.exchange()
.expectStatus().isEqualTo(HttpStatus.CONFLICT)
.expectBody(ProblemDetail.class)
.value(problemDetail -> {
assertEquals("Conflict", problemDetail.getTitle());
assertEquals("Conflict detected.",
problemDetail.getDetail());
});
}
@TestConfiguration
static class TestConfig {
@ -156,6 +173,10 @@ class I18nExceptionTest {
throw new GeneralException("Something went wrong");
}
@PutMapping("/conflict-error")
ResponseEntity<String> throwConflictException() {
throw new ConcurrencyFailureException("Conflict detected");
}
}
}

View File

@ -12,7 +12,7 @@ spring:
mode: always
platform: h2
messages:
basename: config.i18n.messages
basename: config.i18n.messages
halo:
work-dir: ${user.home}/halo-next-test

View File

@ -3,4 +3,7 @@ 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
problemDetail.title.internalServerError=Internal Server Error
problemDetail.title.conflict=Conflict
problemDetail.conflict=Conflict detected.