From 854f13df692a026f21ef162c50dafb12c6234862 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 14 Sep 2026 11:09:33 -0700 Subject: [PATCH 01/10] Return protocol error bodies for HTTP failures. --- .../io/opensharing/http/ApiException.java | 52 +++++++++ .../java/io/opensharing/http/ApiFailure.java | 107 ++++++++++++++++++ .../java/io/opensharing/http/ErrorCodes.java | 18 +++ .../io/opensharing/http/ErrorResponse.java | 4 + .../http/GlobalExceptionHandler.java | 17 +++ .../io/opensharing/http/ApiFailureTest.java | 62 ++++++++++ .../http/GlobalExceptionHandlerTest.java | 31 +++++ 7 files changed, 291 insertions(+) create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorCodes.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java create mode 100644 server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java create mode 100644 server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java new file mode 100644 index 000000000..71a7fb199 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java @@ -0,0 +1,52 @@ +package io.opensharing.http; + +import org.springframework.http.HttpStatus; + +/** An error with an explicit protocol error code and HTTP status. */ +public class ApiException extends RuntimeException { + + private final HttpStatus status; + private final String errorCode; + + public ApiException(HttpStatus status, String errorCode, String message) { + super(message); + this.status = status; + this.errorCode = errorCode; + } + + public HttpStatus getStatus() { + return status; + } + + public String getErrorCode() { + return errorCode; + } + + public static ApiException notFound(String message) { + return new ApiException(HttpStatus.NOT_FOUND, ErrorCodes.RESOURCE_DOES_NOT_EXIST, message); + } + + public static ApiException alreadyExists(String message) { + return new ApiException(HttpStatus.CONFLICT, ErrorCodes.RESOURCE_ALREADY_EXISTS, message); + } + + public static ApiException conflict(String message) { + return new ApiException(HttpStatus.CONFLICT, ErrorCodes.RESOURCE_CONFLICT, message); + } + + public static ApiException invalidParameter(String message) { + return new ApiException(HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, message); + } + + public static ApiException permissionDenied(String message) { + return new ApiException(HttpStatus.FORBIDDEN, ErrorCodes.PERMISSION_DENIED, message); + } + + public static ApiException unauthenticated(String message) { + return new ApiException(HttpStatus.UNAUTHORIZED, ErrorCodes.UNAUTHENTICATED, message); + } + + public static ApiException notImplemented(String message) { + return new ApiException(HttpStatus.NOT_IMPLEMENTED, ErrorCodes.NOT_IMPLEMENTED, message); + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java new file mode 100644 index 000000000..5a86f61a8 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java @@ -0,0 +1,107 @@ +package io.opensharing.http; + +import io.opensharing.exception.AssetAccessDeniedException; +import io.opensharing.exception.AssetNotFoundException; +import io.opensharing.exception.CatalogAuthenticationException; +import io.opensharing.exception.CatalogException; +import io.opensharing.exception.UnsupportedAssetTypeException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.http.HttpStatus; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.MissingServletRequestParameterException; +import org.springframework.web.method.annotation.HandlerMethodValidationException; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; +import org.springframework.web.servlet.resource.NoResourceFoundException; + +/** Maps a failure to an HTTP status, protocol error code, and caller-facing message. */ +public record ApiFailure(HttpStatus status, String errorCode, String message) { + + private static final Logger log = LoggerFactory.getLogger(ApiFailure.class); + + public static ApiFailure of(Exception e) { + return switch (e) { + case ApiException api -> + new ApiFailure(api.getStatus(), api.getErrorCode(), api.getMessage()); + case AssetNotFoundException missing -> + new ApiFailure( + HttpStatus.NOT_FOUND, ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.getMessage()); + case AssetAccessDeniedException denied -> + new ApiFailure(HttpStatus.FORBIDDEN, ErrorCodes.PERMISSION_DENIED, denied.getMessage()); + case UnsupportedAssetTypeException unsupported -> + new ApiFailure( + HttpStatus.BAD_REQUEST, + ErrorCodes.INVALID_PARAMETER_VALUE, + unsupported.getMessage()); + case CatalogAuthenticationException rejected -> { + log.error("The sharing server could not authenticate to the catalog", rejected); + yield new ApiFailure( + HttpStatus.BAD_GATEWAY, + ErrorCodes.CATALOG_ERROR, + "the sharing server could not authenticate to the catalog"); + } + case CatalogException failed -> { + log.error("Catalog request failed", failed); + yield new ApiFailure( + HttpStatus.BAD_GATEWAY, ErrorCodes.CATALOG_ERROR, failed.getMessage()); + } + case MethodArgumentNotValidException invalid -> + new ApiFailure( + HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, describe(invalid)); + case HandlerMethodValidationException ignored -> validationFailed(); + case MethodArgumentTypeMismatchException mistyped -> + new ApiFailure( + HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, describe(mistyped)); + case MissingServletRequestParameterException missing -> + new ApiFailure( + HttpStatus.BAD_REQUEST, + ErrorCodes.INVALID_PARAMETER_VALUE, + missing.getParameterName() + " is required"); + case HttpMessageNotReadableException ignored -> + new ApiFailure( + HttpStatus.BAD_REQUEST, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); + case DataIntegrityViolationException conflict -> { + log.debug("Rejected request that violated a uniqueness constraint", conflict); + yield new ApiFailure( + HttpStatus.CONFLICT, + ErrorCodes.RESOURCE_ALREADY_EXISTS, + "the object already exists or conflicts with an existing object"); + } + case NoResourceFoundException ignored -> + new ApiFailure( + HttpStatus.NOT_FOUND, + ErrorCodes.RESOURCE_DOES_NOT_EXIST, + "the endpoint does not exist"); + case IllegalArgumentException illegal -> + new ApiFailure( + HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, illegal.getMessage()); + default -> { + log.error("Unhandled server error", e); + yield new ApiFailure( + HttpStatus.INTERNAL_SERVER_ERROR, ErrorCodes.INTERNAL_ERROR, "internal server error"); + } + }; + } + + private static ApiFailure validationFailed() { + return new ApiFailure( + HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, "request validation failed"); + } + + private static String describe(MethodArgumentNotValidException invalid) { + return invalid.getBindingResult().getFieldErrors().stream() + .findFirst() + .map(error -> error.getField() + " " + error.getDefaultMessage()) + .orElse("request validation failed"); + } + + private static String describe(MethodArgumentTypeMismatchException mistyped) { + Class wanted = mistyped.getRequiredType(); + boolean numeric = wanted == Integer.class || wanted == Long.class; + return numeric + ? mistyped.getName() + " must be a number" + : mistyped.getName() + " is not a valid value"; + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorCodes.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorCodes.java new file mode 100644 index 000000000..09614b1cd --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorCodes.java @@ -0,0 +1,18 @@ +package io.opensharing.http; + +/** Error codes used in {@link ErrorResponse#errorCode()}. */ +public final class ErrorCodes { + + public static final String MALFORMED_REQUEST = "MALFORMED_REQUEST"; + public static final String INVALID_PARAMETER_VALUE = "INVALID_PARAMETER_VALUE"; + public static final String UNAUTHENTICATED = "UNAUTHENTICATED"; + public static final String PERMISSION_DENIED = "PERMISSION_DENIED"; + public static final String RESOURCE_DOES_NOT_EXIST = "RESOURCE_DOES_NOT_EXIST"; + public static final String RESOURCE_ALREADY_EXISTS = "RESOURCE_ALREADY_EXISTS"; + public static final String RESOURCE_CONFLICT = "RESOURCE_CONFLICT"; + public static final String CATALOG_ERROR = "CATALOG_ERROR"; + public static final String NOT_IMPLEMENTED = "NOT_IMPLEMENTED"; + public static final String INTERNAL_ERROR = "INTERNAL_ERROR"; + + private ErrorCodes() {} +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java new file mode 100644 index 000000000..aea36ae13 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java @@ -0,0 +1,4 @@ +package io.opensharing.http; + +/** Error body returned by every non-2xx protocol response. */ +public record ErrorResponse(String errorCode, String message) {} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java new file mode 100644 index 000000000..daf2590ae --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java @@ -0,0 +1,17 @@ +package io.opensharing.http; + +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +/** Renders every failure as {@code {errorCode, message}}. */ +@RestControllerAdvice +public class GlobalExceptionHandler { + + @ExceptionHandler(Exception.class) + public ResponseEntity handle(Exception e) { + ApiFailure failure = ApiFailure.of(e); + return ResponseEntity.status(failure.status()) + .body(new ErrorResponse(failure.errorCode(), failure.message())); + } +} diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java new file mode 100644 index 000000000..d445fb5ca --- /dev/null +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -0,0 +1,62 @@ +package io.opensharing.http; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.opensharing.catalog.AssetLookup; +import io.opensharing.catalog.AssetType; +import io.opensharing.catalog.CatalogCaller; +import io.opensharing.exception.AssetAccessDeniedException; +import io.opensharing.exception.AssetNotFoundException; +import io.opensharing.exception.CatalogException; +import io.opensharing.exception.UnsupportedAssetTypeException; +import org.junit.jupiter.api.Test; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +import org.springframework.web.servlet.resource.NoResourceFoundException; + +class ApiFailureTest { + + @Test + void mapsCatalogMissesAndDenials() { + AssetLookup lookup = AssetLookup.of(AssetType.TABLE, "main.sales.missing"); + ApiFailure missing = ApiFailure.of(new AssetNotFoundException(lookup)); + assertEquals(HttpStatus.NOT_FOUND, missing.status()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); + + CatalogCaller bob = CatalogCaller.withBearerToken("bob@example.com", "token"); + ApiFailure denied = ApiFailure.of(new AssetAccessDeniedException(lookup, bob)); + assertEquals(HttpStatus.FORBIDDEN, denied.status()); + assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); + } + + @Test + void mapsCatalogFailuresAsBadGateway() { + ApiFailure failure = ApiFailure.of(new CatalogException("catalog unreachable")); + assertEquals(HttpStatus.BAD_GATEWAY, failure.status()); + assertEquals(ErrorCodes.CATALOG_ERROR, failure.errorCode()); + assertEquals("catalog unreachable", failure.message()); + } + + @Test + void mapsUnsupportedAssetTypesAsBadRequest() { + ApiFailure failure = ApiFailure.of(new UnsupportedAssetTypeException("not a schema")); + assertEquals(HttpStatus.BAD_REQUEST, failure.status()); + assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); + } + + @Test + void mapsUnknownRoutes() { + ApiFailure failure = ApiFailure.of(new NoResourceFoundException(HttpMethod.GET, "/missing")); + assertEquals(HttpStatus.NOT_FOUND, failure.status()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, failure.errorCode()); + assertEquals("the endpoint does not exist", failure.message()); + } + + @Test + void keepsAnExplicitApiException() { + ApiFailure failure = ApiFailure.of(ApiException.unauthenticated("token is missing")); + assertEquals(HttpStatus.UNAUTHORIZED, failure.status()); + assertEquals(ErrorCodes.UNAUTHENTICATED, failure.errorCode()); + assertEquals("token is missing", failure.message()); + } +} diff --git a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java new file mode 100644 index 000000000..182b6b25e --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java @@ -0,0 +1,31 @@ +package io.opensharing.http; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.web.servlet.MockMvc; + +@SpringBootTest( + properties = { + "spring.datasource.url=jdbc:h2:mem:http-errors;DB_CLOSE_DELAY=-1", + "opensharing.catalog.type=local", + "opensharing.catalog.local.file=classpath:local-catalog.yml" + }) +@AutoConfigureMockMvc +class GlobalExceptionHandlerTest { + + @Autowired private MockMvc mvc; + + @Test + void answersUnknownRoutesWithTheProtocolErrorBody() throws Exception { + mvc.perform(get("/no-such-route")) + .andExpect(status().isNotFound()) + .andExpect(jsonPath("$.errorCode").value(ErrorCodes.RESOURCE_DOES_NOT_EXIST)) + .andExpect(jsonPath("$.message").value("the endpoint does not exist")); + } +} From 641c63fea4d556df074cd59d65d589fe8e8a83c5 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Wed, 16 Sep 2026 14:51:53 -0700 Subject: [PATCH 02/10] Use UserContext in HTTP error test --- .../src/test/java/io/opensharing/http/ApiFailureTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index d445fb5ca..c59b75b1c 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -2,9 +2,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; +import io.opensharing.auth.UserContext; import io.opensharing.catalog.AssetLookup; import io.opensharing.catalog.AssetType; -import io.opensharing.catalog.CatalogCaller; import io.opensharing.exception.AssetAccessDeniedException; import io.opensharing.exception.AssetNotFoundException; import io.opensharing.exception.CatalogException; @@ -23,7 +23,7 @@ void mapsCatalogMissesAndDenials() { assertEquals(HttpStatus.NOT_FOUND, missing.status()); assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); - CatalogCaller bob = CatalogCaller.withBearerToken("bob@example.com", "token"); + UserContext bob = new UserContext("bob@example.com", "bob@example.com"); ApiFailure denied = ApiFailure.of(new AssetAccessDeniedException(lookup, bob)); assertEquals(HttpStatus.FORBIDDEN, denied.status()); assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); From cbd5583f75082eb3f6fbf45e9eac17080acf0221 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Fri, 2 Oct 2026 09:08:27 -0700 Subject: [PATCH 03/10] Adapt the API failure test to the revised catalog SPI. --- .../src/test/java/io/opensharing/http/ApiFailureTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index c59b75b1c..736a4b4f6 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -3,7 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import io.opensharing.auth.UserContext; -import io.opensharing.catalog.AssetLookup; +import io.opensharing.catalog.Asset; import io.opensharing.catalog.AssetType; import io.opensharing.exception.AssetAccessDeniedException; import io.opensharing.exception.AssetNotFoundException; @@ -18,12 +18,12 @@ class ApiFailureTest { @Test void mapsCatalogMissesAndDenials() { - AssetLookup lookup = AssetLookup.of(AssetType.TABLE, "main.sales.missing"); + Asset lookup = Asset.of(AssetType.TABLE, "main.sales.missing"); ApiFailure missing = ApiFailure.of(new AssetNotFoundException(lookup)); assertEquals(HttpStatus.NOT_FOUND, missing.status()); assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); - UserContext bob = new UserContext("bob@example.com", "bob@example.com"); + UserContext bob = UserContext.fromUserIdAndName("bob@example.com", "bob@example.com"); ApiFailure denied = ApiFailure.of(new AssetAccessDeniedException(lookup, bob)); assertEquals(HttpStatus.FORBIDDEN, denied.status()); assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); From bb404ffcef4d2c46d12257f859adcb8313a7f349 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 5 Oct 2026 14:23:38 -0700 Subject: [PATCH 04/10] Report unknown enum values in request bodies as invalid parameters. --- .../java/io/opensharing/http/ApiFailure.java | 35 ++++++++++++++++-- .../io/opensharing/http/ApiFailureTest.java | 36 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java index 5a86f61a8..c2a6f9b40 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java @@ -1,10 +1,13 @@ package io.opensharing.http; +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.exc.InvalidFormatException; import io.opensharing.exception.AssetAccessDeniedException; import io.opensharing.exception.AssetNotFoundException; import io.opensharing.exception.CatalogAuthenticationException; import io.opensharing.exception.CatalogException; import io.opensharing.exception.UnsupportedAssetTypeException; +import java.util.Arrays; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.dao.DataIntegrityViolationException; @@ -59,9 +62,7 @@ yield new ApiFailure( HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, missing.getParameterName() + " is required"); - case HttpMessageNotReadableException ignored -> - new ApiFailure( - HttpStatus.BAD_REQUEST, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); + case HttpMessageNotReadableException unreadable -> describe(unreadable); case DataIntegrityViolationException conflict -> { log.debug("Rejected request that violated a uniqueness constraint", conflict); yield new ApiFailure( @@ -97,6 +98,34 @@ private static String describe(MethodArgumentNotValidException invalid) { .orElse("request validation failed"); } + /** An unknown enum value is a bad parameter, not a malformed body. */ + private static ApiFailure describe(HttpMessageNotReadableException unreadable) { + if (unreadable.getCause() instanceof InvalidFormatException invalid + && invalid.getTargetType() != null + && invalid.getTargetType().isEnum()) { + return new ApiFailure( + HttpStatus.BAD_REQUEST, + ErrorCodes.INVALID_PARAMETER_VALUE, + fieldPath(invalid) + + " must be one of " + + Arrays.toString(invalid.getTargetType().getEnumConstants())); + } + return new ApiFailure( + HttpStatus.BAD_REQUEST, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); + } + + private static String fieldPath(InvalidFormatException invalid) { + StringBuilder path = new StringBuilder(); + for (JsonMappingException.Reference reference : invalid.getPath()) { + if (reference.getFieldName() != null) { + path.append(path.isEmpty() ? "" : ".").append(reference.getFieldName()); + } else { + path.append('[').append(reference.getIndex()).append(']'); + } + } + return path.isEmpty() ? "value" : path.toString(); + } + private static String describe(MethodArgumentTypeMismatchException mistyped) { Class wanted = mistyped.getRequiredType(); boolean numeric = wanted == Integer.class || wanted == Long.class; diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index 736a4b4f6..d618346b4 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -1,7 +1,10 @@ package io.opensharing.http; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.exc.InvalidFormatException; import io.opensharing.auth.UserContext; import io.opensharing.catalog.Asset; import io.opensharing.catalog.AssetType; @@ -9,9 +12,12 @@ import io.opensharing.exception.AssetNotFoundException; import io.opensharing.exception.CatalogException; import io.opensharing.exception.UnsupportedAssetTypeException; +import java.util.List; import org.junit.jupiter.api.Test; import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.mock.http.MockHttpInputMessage; import org.springframework.web.servlet.resource.NoResourceFoundException; class ApiFailureTest { @@ -59,4 +65,34 @@ void keepsAnExplicitApiException() { assertEquals(ErrorCodes.UNAUTHENTICATED, failure.errorCode()); assertEquals("token is missing", failure.message()); } + + @Test + void mapsUnknownEnumValuesAsInvalidParameters() { + InvalidFormatException invalid = + assertThrows( + InvalidFormatException.class, + () -> + new ObjectMapper() + .readValue("{\"items\":[{\"type\":\"VIEW\"}]}", Body.class)); + ApiFailure failure = ApiFailure.of(unreadable(invalid)); + assertEquals(HttpStatus.BAD_REQUEST, failure.status()); + assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); + assertEquals("items[0].type must be one of [TABLE, SCHEMA]", failure.message()); + } + + @Test + void mapsOtherUnreadableBodiesAsMalformed() { + ApiFailure failure = ApiFailure.of(unreadable(null)); + assertEquals(HttpStatus.BAD_REQUEST, failure.status()); + assertEquals(ErrorCodes.MALFORMED_REQUEST, failure.errorCode()); + } + + private static HttpMessageNotReadableException unreadable(Throwable cause) { + return new HttpMessageNotReadableException( + "unreadable", cause, new MockHttpInputMessage(new byte[0])); + } + + record Body(List items) {} + + record Item(AssetType type) {} } From 9c52d66f791f6bd4a12dc855662ac45b70db138f Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Mon, 5 Oct 2026 15:57:07 -0700 Subject: [PATCH 05/10] Construct Asset directly now that Asset.of is gone. --- .../src/test/java/io/opensharing/http/ApiFailureTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index d618346b4..b29d1fb30 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -24,7 +24,7 @@ class ApiFailureTest { @Test void mapsCatalogMissesAndDenials() { - Asset lookup = Asset.of(AssetType.TABLE, "main.sales.missing"); + Asset lookup = new Asset(AssetType.TABLE, "main.sales.missing"); ApiFailure missing = ApiFailure.of(new AssetNotFoundException(lookup)); assertEquals(HttpStatus.NOT_FOUND, missing.status()); assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); From 57b280e7093a773a0133fddfdca6591e2d2323c7 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Tue, 6 Oct 2026 11:49:09 -0700 Subject: [PATCH 06/10] Keep HTTP statuses as ints in core and map Spring MVC failures in the standalone server. --- .../io/opensharing/http/ApiException.java | 22 ++-- .../java/io/opensharing/http/ApiFailure.java | 109 ++++++------------ .../http/GlobalExceptionHandler.java | 17 --- .../io/opensharing/http/ApiFailureTest.java | 44 +++---- .../http/GlobalExceptionHandler.java | 61 ++++++++++ .../http/GlobalExceptionHandlerTest.java | 33 ++++++ 6 files changed, 160 insertions(+), 126 deletions(-) delete mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java create mode 100644 server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java index 71a7fb199..f294bab62 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiException.java @@ -1,20 +1,18 @@ package io.opensharing.http; -import org.springframework.http.HttpStatus; - /** An error with an explicit protocol error code and HTTP status. */ public class ApiException extends RuntimeException { - private final HttpStatus status; + private final int status; private final String errorCode; - public ApiException(HttpStatus status, String errorCode, String message) { + public ApiException(int status, String errorCode, String message) { super(message); this.status = status; this.errorCode = errorCode; } - public HttpStatus getStatus() { + public int getStatus() { return status; } @@ -23,30 +21,30 @@ public String getErrorCode() { } public static ApiException notFound(String message) { - return new ApiException(HttpStatus.NOT_FOUND, ErrorCodes.RESOURCE_DOES_NOT_EXIST, message); + return new ApiException(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, message); } public static ApiException alreadyExists(String message) { - return new ApiException(HttpStatus.CONFLICT, ErrorCodes.RESOURCE_ALREADY_EXISTS, message); + return new ApiException(409, ErrorCodes.RESOURCE_ALREADY_EXISTS, message); } public static ApiException conflict(String message) { - return new ApiException(HttpStatus.CONFLICT, ErrorCodes.RESOURCE_CONFLICT, message); + return new ApiException(409, ErrorCodes.RESOURCE_CONFLICT, message); } public static ApiException invalidParameter(String message) { - return new ApiException(HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, message); + return new ApiException(400, ErrorCodes.INVALID_PARAMETER_VALUE, message); } public static ApiException permissionDenied(String message) { - return new ApiException(HttpStatus.FORBIDDEN, ErrorCodes.PERMISSION_DENIED, message); + return new ApiException(403, ErrorCodes.PERMISSION_DENIED, message); } public static ApiException unauthenticated(String message) { - return new ApiException(HttpStatus.UNAUTHORIZED, ErrorCodes.UNAUTHENTICATED, message); + return new ApiException(401, ErrorCodes.UNAUTHENTICATED, message); } public static ApiException notImplemented(String message) { - return new ApiException(HttpStatus.NOT_IMPLEMENTED, ErrorCodes.NOT_IMPLEMENTED, message); + return new ApiException(501, ErrorCodes.NOT_IMPLEMENTED, message); } } diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java index c2a6f9b40..c1fdcf36c 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java @@ -1,5 +1,6 @@ package io.opensharing.http; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.exc.InvalidFormatException; import io.opensharing.exception.AssetAccessDeniedException; @@ -7,20 +8,16 @@ import io.opensharing.exception.CatalogAuthenticationException; import io.opensharing.exception.CatalogException; import io.opensharing.exception.UnsupportedAssetTypeException; +import java.sql.SQLException; import java.util.Arrays; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.springframework.dao.DataIntegrityViolationException; -import org.springframework.http.HttpStatus; -import org.springframework.http.converter.HttpMessageNotReadableException; -import org.springframework.web.bind.MethodArgumentNotValidException; -import org.springframework.web.bind.MissingServletRequestParameterException; -import org.springframework.web.method.annotation.HandlerMethodValidationException; -import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; -import org.springframework.web.servlet.resource.NoResourceFoundException; -/** Maps a failure to an HTTP status, protocol error code, and caller-facing message. */ -public record ApiFailure(HttpStatus status, String errorCode, String message) { +/** + * Maps a failure to an HTTP status, protocol error code, and caller-facing message. Hosts map + * their own framework's failures first and pass everything else here. + */ +public record ApiFailure(int status, String errorCode, String message) { private static final Logger log = LoggerFactory.getLogger(ApiFailure.class); @@ -29,89 +26,52 @@ public static ApiFailure of(Exception e) { case ApiException api -> new ApiFailure(api.getStatus(), api.getErrorCode(), api.getMessage()); case AssetNotFoundException missing -> - new ApiFailure( - HttpStatus.NOT_FOUND, ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.getMessage()); + new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.getMessage()); case AssetAccessDeniedException denied -> - new ApiFailure(HttpStatus.FORBIDDEN, ErrorCodes.PERMISSION_DENIED, denied.getMessage()); + new ApiFailure(403, ErrorCodes.PERMISSION_DENIED, denied.getMessage()); case UnsupportedAssetTypeException unsupported -> - new ApiFailure( - HttpStatus.BAD_REQUEST, - ErrorCodes.INVALID_PARAMETER_VALUE, - unsupported.getMessage()); + new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, unsupported.getMessage()); case CatalogAuthenticationException rejected -> { log.error("The sharing server could not authenticate to the catalog", rejected); yield new ApiFailure( - HttpStatus.BAD_GATEWAY, + 502, ErrorCodes.CATALOG_ERROR, "the sharing server could not authenticate to the catalog"); } case CatalogException failed -> { log.error("Catalog request failed", failed); - yield new ApiFailure( - HttpStatus.BAD_GATEWAY, ErrorCodes.CATALOG_ERROR, failed.getMessage()); - } - case MethodArgumentNotValidException invalid -> - new ApiFailure( - HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, describe(invalid)); - case HandlerMethodValidationException ignored -> validationFailed(); - case MethodArgumentTypeMismatchException mistyped -> - new ApiFailure( - HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, describe(mistyped)); - case MissingServletRequestParameterException missing -> - new ApiFailure( - HttpStatus.BAD_REQUEST, - ErrorCodes.INVALID_PARAMETER_VALUE, - missing.getParameterName() + " is required"); - case HttpMessageNotReadableException unreadable -> describe(unreadable); - case DataIntegrityViolationException conflict -> { - log.debug("Rejected request that violated a uniqueness constraint", conflict); - yield new ApiFailure( - HttpStatus.CONFLICT, - ErrorCodes.RESOURCE_ALREADY_EXISTS, - "the object already exists or conflicts with an existing object"); + yield new ApiFailure(502, ErrorCodes.CATALOG_ERROR, failed.getMessage()); } - case NoResourceFoundException ignored -> - new ApiFailure( - HttpStatus.NOT_FOUND, - ErrorCodes.RESOURCE_DOES_NOT_EXIST, - "the endpoint does not exist"); + case JsonProcessingException unreadable -> describe(unreadable); case IllegalArgumentException illegal -> - new ApiFailure( - HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, illegal.getMessage()); + new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, illegal.getMessage()); default -> { + if (violatesConstraint(e)) { + log.debug("Rejected request that violated a uniqueness constraint", e); + yield new ApiFailure( + 409, + ErrorCodes.RESOURCE_ALREADY_EXISTS, + "the object already exists or conflicts with an existing object"); + } log.error("Unhandled server error", e); - yield new ApiFailure( - HttpStatus.INTERNAL_SERVER_ERROR, ErrorCodes.INTERNAL_ERROR, "internal server error"); + yield new ApiFailure(500, ErrorCodes.INTERNAL_ERROR, "internal server error"); } }; } - private static ApiFailure validationFailed() { - return new ApiFailure( - HttpStatus.BAD_REQUEST, ErrorCodes.INVALID_PARAMETER_VALUE, "request validation failed"); - } - - private static String describe(MethodArgumentNotValidException invalid) { - return invalid.getBindingResult().getFieldErrors().stream() - .findFirst() - .map(error -> error.getField() + " " + error.getDefaultMessage()) - .orElse("request validation failed"); - } - /** An unknown enum value is a bad parameter, not a malformed body. */ - private static ApiFailure describe(HttpMessageNotReadableException unreadable) { - if (unreadable.getCause() instanceof InvalidFormatException invalid + private static ApiFailure describe(JsonProcessingException unreadable) { + if (unreadable instanceof InvalidFormatException invalid && invalid.getTargetType() != null && invalid.getTargetType().isEnum()) { return new ApiFailure( - HttpStatus.BAD_REQUEST, + 400, ErrorCodes.INVALID_PARAMETER_VALUE, fieldPath(invalid) + " must be one of " + Arrays.toString(invalid.getTargetType().getEnumConstants())); } - return new ApiFailure( - HttpStatus.BAD_REQUEST, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); + return new ApiFailure(400, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); } private static String fieldPath(InvalidFormatException invalid) { @@ -126,11 +86,16 @@ private static String fieldPath(InvalidFormatException invalid) { return path.isEmpty() ? "value" : path.toString(); } - private static String describe(MethodArgumentTypeMismatchException mistyped) { - Class wanted = mistyped.getRequiredType(); - boolean numeric = wanted == Integer.class || wanted == Long.class; - return numeric - ? mistyped.getName() + " must be a number" - : mistyped.getName() + " is not a valid value"; + // SQLSTATE class 23 is an integrity constraint violation in every database. Hosts wrap it in + // their own persistence exceptions, so look for it anywhere in the cause chain. + private static boolean violatesConstraint(Throwable e) { + for (Throwable cause = e; cause != null; cause = cause.getCause()) { + if (cause instanceof SQLException sql + && sql.getSQLState() != null + && sql.getSQLState().startsWith("23")) { + return true; + } + } + return false; } } diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java deleted file mode 100644 index daf2590ae..000000000 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/GlobalExceptionHandler.java +++ /dev/null @@ -1,17 +0,0 @@ -package io.opensharing.http; - -import org.springframework.http.ResponseEntity; -import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.bind.annotation.RestControllerAdvice; - -/** Renders every failure as {@code {errorCode, message}}. */ -@RestControllerAdvice -public class GlobalExceptionHandler { - - @ExceptionHandler(Exception.class) - public ResponseEntity handle(Exception e) { - ApiFailure failure = ApiFailure.of(e); - return ResponseEntity.status(failure.status()) - .body(new ErrorResponse(failure.errorCode(), failure.message())); - } -} diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index b29d1fb30..74dc74689 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -12,13 +12,10 @@ import io.opensharing.exception.AssetNotFoundException; import io.opensharing.exception.CatalogException; import io.opensharing.exception.UnsupportedAssetTypeException; +import com.fasterxml.jackson.core.JsonProcessingException; +import java.sql.SQLException; import java.util.List; import org.junit.jupiter.api.Test; -import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatus; -import org.springframework.http.converter.HttpMessageNotReadableException; -import org.springframework.mock.http.MockHttpInputMessage; -import org.springframework.web.servlet.resource.NoResourceFoundException; class ApiFailureTest { @@ -26,19 +23,19 @@ class ApiFailureTest { void mapsCatalogMissesAndDenials() { Asset lookup = new Asset(AssetType.TABLE, "main.sales.missing"); ApiFailure missing = ApiFailure.of(new AssetNotFoundException(lookup)); - assertEquals(HttpStatus.NOT_FOUND, missing.status()); + assertEquals(404, missing.status()); assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); UserContext bob = UserContext.fromUserIdAndName("bob@example.com", "bob@example.com"); ApiFailure denied = ApiFailure.of(new AssetAccessDeniedException(lookup, bob)); - assertEquals(HttpStatus.FORBIDDEN, denied.status()); + assertEquals(403, denied.status()); assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); } @Test void mapsCatalogFailuresAsBadGateway() { ApiFailure failure = ApiFailure.of(new CatalogException("catalog unreachable")); - assertEquals(HttpStatus.BAD_GATEWAY, failure.status()); + assertEquals(502, failure.status()); assertEquals(ErrorCodes.CATALOG_ERROR, failure.errorCode()); assertEquals("catalog unreachable", failure.message()); } @@ -46,22 +43,14 @@ void mapsCatalogFailuresAsBadGateway() { @Test void mapsUnsupportedAssetTypesAsBadRequest() { ApiFailure failure = ApiFailure.of(new UnsupportedAssetTypeException("not a schema")); - assertEquals(HttpStatus.BAD_REQUEST, failure.status()); + assertEquals(400, failure.status()); assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); } - @Test - void mapsUnknownRoutes() { - ApiFailure failure = ApiFailure.of(new NoResourceFoundException(HttpMethod.GET, "/missing")); - assertEquals(HttpStatus.NOT_FOUND, failure.status()); - assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, failure.errorCode()); - assertEquals("the endpoint does not exist", failure.message()); - } - @Test void keepsAnExplicitApiException() { ApiFailure failure = ApiFailure.of(ApiException.unauthenticated("token is missing")); - assertEquals(HttpStatus.UNAUTHORIZED, failure.status()); + assertEquals(401, failure.status()); assertEquals(ErrorCodes.UNAUTHENTICATED, failure.errorCode()); assertEquals("token is missing", failure.message()); } @@ -74,22 +63,27 @@ void mapsUnknownEnumValuesAsInvalidParameters() { () -> new ObjectMapper() .readValue("{\"items\":[{\"type\":\"VIEW\"}]}", Body.class)); - ApiFailure failure = ApiFailure.of(unreadable(invalid)); - assertEquals(HttpStatus.BAD_REQUEST, failure.status()); + ApiFailure failure = ApiFailure.of(invalid); + assertEquals(400, failure.status()); assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); assertEquals("items[0].type must be one of [TABLE, SCHEMA]", failure.message()); } @Test void mapsOtherUnreadableBodiesAsMalformed() { - ApiFailure failure = ApiFailure.of(unreadable(null)); - assertEquals(HttpStatus.BAD_REQUEST, failure.status()); + JsonProcessingException malformed = + assertThrows(JsonProcessingException.class, () -> new ObjectMapper().readTree("{")); + ApiFailure failure = ApiFailure.of(malformed); + assertEquals(400, failure.status()); assertEquals(ErrorCodes.MALFORMED_REQUEST, failure.errorCode()); } - private static HttpMessageNotReadableException unreadable(Throwable cause) { - return new HttpMessageNotReadableException( - "unreadable", cause, new MockHttpInputMessage(new byte[0])); + @Test + void mapsConstraintViolationsAsAlreadyExists() { + SQLException duplicate = new SQLException("duplicate key", "23505"); + ApiFailure failure = ApiFailure.of(new RuntimeException("commit failed", duplicate)); + assertEquals(409, failure.status()); + assertEquals(ErrorCodes.RESOURCE_ALREADY_EXISTS, failure.errorCode()); } record Body(List items) {} diff --git a/server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java b/server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java new file mode 100644 index 000000000..b1b0c052e --- /dev/null +++ b/server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java @@ -0,0 +1,61 @@ +package io.opensharing.http; + +import com.fasterxml.jackson.core.JsonProcessingException; +import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.MissingServletRequestParameterException; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.method.annotation.HandlerMethodValidationException; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; +import org.springframework.web.servlet.resource.NoResourceFoundException; + +/** Renders every failure as {@code {errorCode, message}}. */ +@RestControllerAdvice +public class GlobalExceptionHandler { + + @ExceptionHandler(Exception.class) + public ResponseEntity handle(Exception e) { + ApiFailure failure = failureOf(e); + return ResponseEntity.status(failure.status()) + .body(new ErrorResponse(failure.errorCode(), failure.message())); + } + + /** Maps Spring MVC's own failures; everything else is mapped by {@link ApiFailure}. */ + static ApiFailure failureOf(Exception e) { + return switch (e) { + case MethodArgumentNotValidException invalid -> + new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, describe(invalid)); + case HandlerMethodValidationException ignored -> + new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, "request validation failed"); + case MethodArgumentTypeMismatchException mistyped -> + new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, describe(mistyped)); + case MissingServletRequestParameterException missing -> + new ApiFailure( + 400, ErrorCodes.INVALID_PARAMETER_VALUE, missing.getParameterName() + " is required"); + case HttpMessageNotReadableException unreadable -> + unreadable.getCause() instanceof JsonProcessingException json + ? ApiFailure.of(json) + : new ApiFailure(400, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); + case NoResourceFoundException ignored -> + new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "the endpoint does not exist"); + default -> ApiFailure.of(e); + }; + } + + private static String describe(MethodArgumentNotValidException invalid) { + return invalid.getBindingResult().getFieldErrors().stream() + .findFirst() + .map(error -> error.getField() + " " + error.getDefaultMessage()) + .orElse("request validation failed"); + } + + private static String describe(MethodArgumentTypeMismatchException mistyped) { + Class wanted = mistyped.getRequiredType(); + boolean numeric = wanted == Integer.class || wanted == Long.class; + return numeric + ? mistyped.getName() + " must be a number" + : mistyped.getName() + " is not a valid value"; + } +} diff --git a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java index 182b6b25e..825b17a42 100644 --- a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java +++ b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java @@ -1,13 +1,20 @@ package io.opensharing.http; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.exc.InvalidFormatException; +import io.opensharing.catalog.AssetType; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.mock.http.MockHttpInputMessage; import org.springframework.test.web.servlet.MockMvc; @SpringBootTest( @@ -28,4 +35,30 @@ void answersUnknownRoutesWithTheProtocolErrorBody() throws Exception { .andExpect(jsonPath("$.errorCode").value(ErrorCodes.RESOURCE_DOES_NOT_EXIST)) .andExpect(jsonPath("$.message").value("the endpoint does not exist")); } + + @Test + void mapsUnknownEnumValuesInABodyAsInvalidParameters() { + InvalidFormatException invalid = + assertThrows( + InvalidFormatException.class, + () -> new ObjectMapper().readValue("{\"type\":\"VIEW\"}", Body.class)); + ApiFailure failure = GlobalExceptionHandler.failureOf(unreadable(invalid)); + assertEquals(400, failure.status()); + assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); + assertEquals("type must be one of [TABLE, SCHEMA]", failure.message()); + } + + @Test + void mapsOtherUnreadableBodiesAsMalformed() { + ApiFailure failure = GlobalExceptionHandler.failureOf(unreadable(null)); + assertEquals(400, failure.status()); + assertEquals(ErrorCodes.MALFORMED_REQUEST, failure.errorCode()); + } + + private static HttpMessageNotReadableException unreadable(Throwable cause) { + return new HttpMessageNotReadableException( + "unreadable", cause, new MockHttpInputMessage(new byte[0])); + } + + record Body(AssetType type) {} } From 49c90d03b67eb9ef5358f5fddde1e3b31ff7caf1 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Wed, 7 Oct 2026 18:07:38 -0700 Subject: [PATCH 07/10] Test every failure mapping in ApiFailure and GlobalExceptionHandler. --- .../java/io/opensharing/http/ApiFailure.java | 5 + .../io/opensharing/http/ApiFailureTest.java | 168 +++++++++++++----- .../http/GlobalExceptionHandlerTest.java | 84 +++++++-- 3 files changed, 204 insertions(+), 53 deletions(-) diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java index c1fdcf36c..37d38df47 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java @@ -74,9 +74,14 @@ private static ApiFailure describe(JsonProcessingException unreadable) { return new ApiFailure(400, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); } + /** + * The JSON path of the invalid value, such as {@code a.b[0].c}, or {@code value} + * when the body itself is the invalid value. + */ private static String fieldPath(InvalidFormatException invalid) { StringBuilder path = new StringBuilder(); for (JsonMappingException.Reference reference : invalid.getPath()) { + // A reference names an object field, or has a null field name and indexes into an array. if (reference.getFieldName() != null) { path.append(path.isEmpty() ? "" : ".").append(reference.getFieldName()); } else { diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index 74dc74689..a3dc2767f 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.exc.InvalidFormatException; import io.opensharing.auth.UserContext; @@ -10,83 +11,168 @@ import io.opensharing.catalog.AssetType; import io.opensharing.exception.AssetAccessDeniedException; import io.opensharing.exception.AssetNotFoundException; +import io.opensharing.exception.CatalogAuthenticationException; +import io.opensharing.exception.CatalogAuthorizationException; import io.opensharing.exception.CatalogException; import io.opensharing.exception.UnsupportedAssetTypeException; -import com.fasterxml.jackson.core.JsonProcessingException; import java.sql.SQLException; import java.util.List; import org.junit.jupiter.api.Test; class ApiFailureTest { + private static final Asset MISSING = new Asset(AssetType.TABLE, "main.sales.missing"); + @Test - void mapsCatalogMissesAndDenials() { - Asset lookup = new Asset(AssetType.TABLE, "main.sales.missing"); - ApiFailure missing = ApiFailure.of(new AssetNotFoundException(lookup)); - assertEquals(404, missing.status()); - assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, missing.errorCode()); + void keepsAnExplicitApiException() { + assertFailure( + 401, + ErrorCodes.UNAUTHENTICATED, + "token is missing", + ApiException.unauthenticated("token is missing")); + assertFailure( + 501, ErrorCodes.NOT_IMPLEMENTED, "not yet", ApiException.notImplemented("not yet")); + } - UserContext bob = UserContext.fromUserIdAndName("bob@example.com", "bob@example.com"); - ApiFailure denied = ApiFailure.of(new AssetAccessDeniedException(lookup, bob)); - assertEquals(403, denied.status()); - assertEquals(ErrorCodes.PERMISSION_DENIED, denied.errorCode()); + @Test + void mapsMissingAssetsAsNotFound() { + assertFailure( + 404, + ErrorCodes.RESOURCE_DOES_NOT_EXIST, + "TABLE 'main.sales.missing' does not exist in the catalog", + new AssetNotFoundException(MISSING)); } @Test - void mapsCatalogFailuresAsBadGateway() { - ApiFailure failure = ApiFailure.of(new CatalogException("catalog unreachable")); - assertEquals(502, failure.status()); - assertEquals(ErrorCodes.CATALOG_ERROR, failure.errorCode()); - assertEquals("catalog unreachable", failure.message()); + void mapsDeniedAssetsAsPermissionDenied() { + UserContext bob = UserContext.fromUserIdAndName("bob@example.com", "bob@example.com"); + assertFailure( + 403, + ErrorCodes.PERMISSION_DENIED, + "'bob@example.com' may not share TABLE 'main.sales.missing'", + new AssetAccessDeniedException(MISSING, bob)); } @Test void mapsUnsupportedAssetTypesAsBadRequest() { - ApiFailure failure = ApiFailure.of(new UnsupportedAssetTypeException("not a schema")); - assertEquals(400, failure.status()); - assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); + assertFailure( + 400, + ErrorCodes.INVALID_PARAMETER_VALUE, + "not a schema", + new UnsupportedAssetTypeException("not a schema")); } @Test - void keepsAnExplicitApiException() { - ApiFailure failure = ApiFailure.of(ApiException.unauthenticated("token is missing")); - assertEquals(401, failure.status()); - assertEquals(ErrorCodes.UNAUTHENTICATED, failure.errorCode()); - assertEquals("token is missing", failure.message()); + void hidesCatalogAuthenticationFailuresBehindBadGateway() { + assertFailure( + 502, + ErrorCodes.CATALOG_ERROR, + "the sharing server could not authenticate to the catalog", + new CatalogAuthenticationException("token for svc-sharing expired")); + } + + @Test + void mapsOtherCatalogFailuresAsBadGateway() { + assertFailure( + 502, + ErrorCodes.CATALOG_ERROR, + "catalog unreachable", + new CatalogException("catalog unreachable")); + assertFailure( + 502, + ErrorCodes.CATALOG_ERROR, + "user lacks USE CATALOG", + new CatalogAuthorizationException("user lacks USE CATALOG")); } @Test void mapsUnknownEnumValuesAsInvalidParameters() { - InvalidFormatException invalid = - assertThrows( - InvalidFormatException.class, - () -> - new ObjectMapper() - .readValue("{\"items\":[{\"type\":\"VIEW\"}]}", Body.class)); - ApiFailure failure = ApiFailure.of(invalid); - assertEquals(400, failure.status()); - assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); - assertEquals("items[0].type must be one of [TABLE, SCHEMA]", failure.message()); + assertFailure( + 400, + ErrorCodes.INVALID_PARAMETER_VALUE, + "items[0].type must be one of [TABLE, SCHEMA]", + unreadable("{\"items\":[{\"type\":\"VIEW\"}]}", Body.class)); + } + + @Test + void namesATopLevelEnumValueAsValue() { + assertFailure( + 400, + ErrorCodes.INVALID_PARAMETER_VALUE, + "value must be one of [TABLE, SCHEMA]", + unreadable("\"VIEW\"", AssetType.class)); + } + + @Test + void mapsOtherInvalidFormatsAsMalformed() { + InvalidFormatException invalid = unreadable("{\"count\":\"ten\"}", Counted.class); + assertFailure(400, ErrorCodes.MALFORMED_REQUEST, "request body is malformed", invalid); } @Test - void mapsOtherUnreadableBodiesAsMalformed() { + void mapsUnparsableBodiesAsMalformed() { JsonProcessingException malformed = assertThrows(JsonProcessingException.class, () -> new ObjectMapper().readTree("{")); - ApiFailure failure = ApiFailure.of(malformed); - assertEquals(400, failure.status()); - assertEquals(ErrorCodes.MALFORMED_REQUEST, failure.errorCode()); + assertFailure(400, ErrorCodes.MALFORMED_REQUEST, "request body is malformed", malformed); + } + + @Test + void mapsIllegalArgumentsAsInvalidParameters() { + assertFailure( + 400, + ErrorCodes.INVALID_PARAMETER_VALUE, + "name must not be blank", + new IllegalArgumentException("name must not be blank")); } @Test - void mapsConstraintViolationsAsAlreadyExists() { + void mapsConstraintViolationsAnywhereInTheCauseChainAsAlreadyExists() { SQLException duplicate = new SQLException("duplicate key", "23505"); - ApiFailure failure = ApiFailure.of(new RuntimeException("commit failed", duplicate)); - assertEquals(409, failure.status()); - assertEquals(ErrorCodes.RESOURCE_ALREADY_EXISTS, failure.errorCode()); + assertFailure( + 409, + ErrorCodes.RESOURCE_ALREADY_EXISTS, + "the object already exists or conflicts with an existing object", + new RuntimeException("commit failed", new IllegalStateException("rollback", duplicate))); + } + + @Test + void mapsOtherSqlFailuresAsInternalErrors() { + assertFailure( + 500, + ErrorCodes.INTERNAL_ERROR, + "internal server error", + new RuntimeException(new SQLException("connection lost", "08006"))); + assertFailure( + 500, + ErrorCodes.INTERNAL_ERROR, + "internal server error", + new RuntimeException(new SQLException("no state"))); + } + + @Test + void hidesUnhandledFailuresBehindInternalError() { + assertFailure( + 500, + ErrorCodes.INTERNAL_ERROR, + "internal server error", + new IllegalStateException("secret detail")); + } + + private static void assertFailure(int status, String errorCode, String message, Exception e) { + ApiFailure failure = ApiFailure.of(e); + assertEquals(status, failure.status()); + assertEquals(errorCode, failure.errorCode()); + assertEquals(message, failure.message()); + } + + private static InvalidFormatException unreadable(String json, Class type) { + return assertThrows( + InvalidFormatException.class, () -> new ObjectMapper().readValue(json, type)); } record Body(List items) {} record Item(AssetType type) {} + + record Counted(int count) {} } diff --git a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java index 825b17a42..8b36f1a67 100644 --- a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java +++ b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java @@ -9,13 +9,24 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.exc.InvalidFormatException; import io.opensharing.catalog.AssetType; +import java.util.List; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.support.DefaultMessageSourceResolvable; +import org.springframework.core.MethodParameter; import org.springframework.http.converter.HttpMessageNotReadableException; import org.springframework.mock.http.MockHttpInputMessage; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.validation.BeanPropertyBindingResult; +import org.springframework.validation.FieldError; +import org.springframework.validation.method.MethodValidationResult; +import org.springframework.validation.method.ParameterValidationResult; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.MissingServletRequestParameterException; +import org.springframework.web.method.annotation.HandlerMethodValidationException; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; @SpringBootTest( properties = { @@ -37,28 +48,77 @@ void answersUnknownRoutesWithTheProtocolErrorBody() throws Exception { } @Test - void mapsUnknownEnumValuesInABodyAsInvalidParameters() { + void mapsInvalidBodiesWithTheFirstFieldError() throws Exception { + BeanPropertyBindingResult result = new BeanPropertyBindingResult(new Body(null), "body"); + result.addError(new FieldError("body", "type", "must not be null")); + assertFailure( + ErrorCodes.INVALID_PARAMETER_VALUE, + "type must not be null", + new MethodArgumentNotValidException(maxResults(), result)); + } + + @Test + void mapsMethodValidationFailures() throws Exception { + MethodParameter maxResults = maxResults(); + ParameterValidationResult invalid = + new ParameterValidationResult( + maxResults, + 0, + List.of(new DefaultMessageSourceResolvable(null, null, "must be greater than 0"))); + assertFailure( + ErrorCodes.INVALID_PARAMETER_VALUE, + "request validation failed", + new HandlerMethodValidationException( + MethodValidationResult.create(new Object(), maxResults.getMethod(), List.of(invalid)))); + } + + @Test + void mapsMistypedParameters() throws Exception { + assertFailure( + ErrorCodes.INVALID_PARAMETER_VALUE, + "maxResults must be a number", + new MethodArgumentTypeMismatchException( + "ten", Integer.class, "maxResults", maxResults(), null)); + } + + @Test + void mapsMissingParameters() { + assertFailure( + ErrorCodes.INVALID_PARAMETER_VALUE, + "maxResults is required", + new MissingServletRequestParameterException("maxResults", "Integer")); + } + + @Test + void mapsUnreadableBodiesThroughTheirJsonCause() { InvalidFormatException invalid = assertThrows( InvalidFormatException.class, () -> new ObjectMapper().readValue("{\"type\":\"VIEW\"}", Body.class)); - ApiFailure failure = GlobalExceptionHandler.failureOf(unreadable(invalid)); - assertEquals(400, failure.status()); - assertEquals(ErrorCodes.INVALID_PARAMETER_VALUE, failure.errorCode()); - assertEquals("type must be one of [TABLE, SCHEMA]", failure.message()); + assertFailure( + ErrorCodes.INVALID_PARAMETER_VALUE, + "type must be one of [TABLE, SCHEMA]", + new HttpMessageNotReadableException( + "unreadable", invalid, new MockHttpInputMessage(new byte[0]))); } @Test - void mapsOtherUnreadableBodiesAsMalformed() { - ApiFailure failure = GlobalExceptionHandler.failureOf(unreadable(null)); - assertEquals(400, failure.status()); - assertEquals(ErrorCodes.MALFORMED_REQUEST, failure.errorCode()); + void leavesOtherFailuresToApiFailure() { + ApiFailure failure = GlobalExceptionHandler.failureOf(ApiException.notFound("no share")); + assertEquals(new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "no share"), failure); } - private static HttpMessageNotReadableException unreadable(Throwable cause) { - return new HttpMessageNotReadableException( - "unreadable", cause, new MockHttpInputMessage(new byte[0])); + private static void assertFailure(String errorCode, String message, Exception e) { + assertEquals(new ApiFailure(400, errorCode, message), GlobalExceptionHandler.failureOf(e)); + } + + private static MethodParameter maxResults() throws NoSuchMethodException { + return new MethodParameter(Endpoint.class.getDeclaredMethod("list", Integer.class), 0); } record Body(AssetType type) {} + + interface Endpoint { + void list(Integer maxResults); + } } From 776d9f462b4bdd3150cfe3912ead8b5b7964bf66 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Tue, 13 Oct 2026 15:04:15 -0700 Subject: [PATCH 08/10] Render protocol error bodies from an Armeria exception handler shared by Armeria hosts. --- server/opensharing-server-core/pom.xml | 8 ++ .../opensharing/http/ApiExceptionHandler.java | 74 +++++++++++ .../java/io/opensharing/http/ApiFailure.java | 3 + .../java/io/opensharing/http/ApiJson.java | 44 +++++++ .../io/opensharing/http/ErrorResponse.java | 8 +- .../http/ApiExceptionHandlerTest.java | 121 +++++++++++++++++ .../io/opensharing/http/ApiFailureTest.java | 12 +- .../io/opensharing/OpenSharingServer.java | 11 +- .../http/GlobalExceptionHandler.java | 61 --------- .../http/GlobalExceptionHandlerTest.java | 124 ------------------ .../io/opensharing/http/UnknownRouteTest.java | 30 +++++ 11 files changed, 303 insertions(+), 193 deletions(-) create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ApiExceptionHandler.java create mode 100644 server/opensharing-server-core/src/main/java/io/opensharing/http/ApiJson.java create mode 100644 server/opensharing-server-core/src/test/java/io/opensharing/http/ApiExceptionHandlerTest.java delete mode 100644 server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java delete mode 100644 server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java create mode 100644 server/opensharing-server/src/test/java/io/opensharing/http/UnknownRouteTest.java diff --git a/server/opensharing-server-core/pom.xml b/server/opensharing-server-core/pom.xml index aeeb03152..f6c0b6ae4 100644 --- a/server/opensharing-server-core/pom.xml +++ b/server/opensharing-server-core/pom.xml @@ -24,6 +24,14 @@ com.fasterxml.jackson.core jackson-databind + + com.fasterxml.jackson.datatype + jackson-datatype-jsr310 + + + com.linecorp.armeria + armeria + org.slf4j slf4j-api diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiExceptionHandler.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiExceptionHandler.java new file mode 100644 index 000000000..770254af2 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiExceptionHandler.java @@ -0,0 +1,74 @@ +package io.opensharing.http; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.ResponseHeaders; +import com.linecorp.armeria.common.ResponseHeadersBuilder; +import com.linecorp.armeria.server.DecoratingHttpServiceFunction; +import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.HttpStatusException; +import com.linecorp.armeria.server.ServiceRequestContext; +import com.linecorp.armeria.server.annotation.ExceptionHandlerFunction; +import java.util.Locale; +import java.util.concurrent.CompletionException; + +/** + * Turns any failure into the protocol's {@code {errorCode, message}} JSON body. Armeria hands it + * failures two ways, so register it twice: as the annotated service's exception handler, for what + * the service methods throw, and as the outermost decorator, for what other decorators such as a + * token check throw, which the exception handler never sees. + */ +public final class ApiExceptionHandler + implements ExceptionHandlerFunction, DecoratingHttpServiceFunction { + + @Override + public HttpResponse handleException( + ServiceRequestContext ctx, HttpRequest req, Throwable cause) { + return response(failureOf(cause)); + } + + @Override + public HttpResponse serve(HttpService delegate, ServiceRequestContext ctx, HttpRequest req) { + try { + return delegate.serve(ctx, req).recover(cause -> handleException(ctx, req, cause)); + } catch (Exception e) { + return handleException(ctx, req, e); + } + } + + /** Maps Armeria's own failures; everything else is mapped by {@link ApiFailure}. */ + static ApiFailure failureOf(Throwable cause) { + Throwable failure = cause instanceof CompletionException ? cause.getCause() : cause; + return switch (failure) { + case HttpStatusException status when status.httpStatus() == HttpStatus.NOT_FOUND -> + new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "the endpoint does not exist"); + case HttpStatusException status -> statusFailure(status.httpStatus()); + // Armeria reports an unreadable body as an IllegalArgumentException around Jackson's failure. + case IllegalArgumentException illegal + when illegal.getCause() instanceof JsonProcessingException json -> + ApiFailure.of(json); + case Exception e -> ApiFailure.of(e); + default -> ApiFailure.of(new RuntimeException(failure)); + }; + } + + private static ApiFailure statusFailure(HttpStatus status) { + String message = status.reasonPhrase().toLowerCase(Locale.ROOT); + return status.isServerError() + ? new ApiFailure(status.code(), ErrorCodes.INTERNAL_ERROR, message) + : new ApiFailure(status.code(), ErrorCodes.MALFORMED_REQUEST, message); + } + + private static HttpResponse response(ApiFailure failure) { + ResponseHeadersBuilder headers = ResponseHeaders.builder(failure.status()); + if (failure.status() == 401) { + // RFC 9110 requires a 401 to name the expected scheme in WWW-Authenticate. + headers.add(HttpHeaderNames.WWW_AUTHENTICATE, "Bearer"); + } + return ApiJson.response( + headers.build(), new ErrorResponse(failure.errorCode(), failure.message())); + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java index 37d38df47..da2637f38 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java @@ -6,6 +6,7 @@ import io.opensharing.exception.AssetAccessDeniedException; import io.opensharing.exception.AssetNotFoundException; import io.opensharing.exception.CatalogAuthenticationException; +import io.opensharing.exception.CatalogAuthorizationException; import io.opensharing.exception.CatalogException; import io.opensharing.exception.UnsupportedAssetTypeException; import java.sql.SQLException; @@ -38,6 +39,8 @@ yield new ApiFailure( ErrorCodes.CATALOG_ERROR, "the sharing server could not authenticate to the catalog"); } + case CatalogAuthorizationException refused -> + new ApiFailure(403, ErrorCodes.PERMISSION_DENIED, refused.getMessage()); case CatalogException failed -> { log.error("Catalog request failed", failed); yield new ApiFailure(502, ErrorCodes.CATALOG_ERROR, failed.getMessage()); diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiJson.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiJson.java new file mode 100644 index 000000000..28b7436d0 --- /dev/null +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiJson.java @@ -0,0 +1,44 @@ +package io.opensharing.http; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.MapperFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.ResponseHeaders; + +/** + * The JSON format of every OpenSharing endpoint, used instead of the host server's own mapper so + * responses look the same in every host: null fields are left out, enum values are read in any + * case, unknown fields are ignored, and dates are ISO-8601 strings. + */ +public final class ApiJson { + + public static final ObjectMapper MAPPER = + JsonMapper.builder() + .serializationInclusion(JsonInclude.Include.NON_NULL) + .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS) + .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) + .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS) + .addModule(new JavaTimeModule()) + .build(); + + private ApiJson() {} + + /** {@code body} as a JSON response with {@code headers}, which set its status. */ + public static HttpResponse response(ResponseHeaders headers, Object body) { + try { + return HttpResponse.of( + headers.toBuilder().contentType(MediaType.JSON_UTF_8).build(), + HttpData.wrap(MAPPER.writeValueAsBytes(body))); + } catch (JsonProcessingException e) { + throw new IllegalStateException("failed to write an OpenSharing response", e); + } + } +} diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java index aea36ae13..cf4dcf395 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ErrorResponse.java @@ -1,4 +1,10 @@ package io.opensharing.http; -/** Error body returned by every non-2xx protocol response. */ +/** + * Error body returned by every non-2xx protocol response: + * + * 
{@code
+ * {"errorCode": "", "message": ""}
+ * }
+ */ public record ErrorResponse(String errorCode, String message) {} diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiExceptionHandlerTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiExceptionHandlerTest.java new file mode 100644 index 000000000..1c099eb00 --- /dev/null +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiExceptionHandlerTest.java @@ -0,0 +1,121 @@ +package io.opensharing.http; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.exc.InvalidFormatException; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.HttpStatusException; +import com.linecorp.armeria.server.ServiceRequestContext; +import io.opensharing.catalog.AssetType; +import java.util.concurrent.CompletionException; +import org.junit.jupiter.api.Test; + +class ApiExceptionHandlerTest { + + private final ApiExceptionHandler handler = new ApiExceptionHandler(); + + @Test + void mapsUnroutedRequestsAsNotFound() { + assertEquals( + new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "the endpoint does not exist"), + ApiExceptionHandler.failureOf(HttpStatusException.of(HttpStatus.NOT_FOUND))); + } + + @Test + void mapsOtherArmeriaStatusesByTheirReason() { + assertEquals( + new ApiFailure(415, ErrorCodes.MALFORMED_REQUEST, "unsupported media type"), + ApiExceptionHandler.failureOf( + HttpStatusException.of(HttpStatus.UNSUPPORTED_MEDIA_TYPE))); + assertEquals( + new ApiFailure(503, ErrorCodes.INTERNAL_ERROR, "service unavailable"), + ApiExceptionHandler.failureOf( + HttpStatusException.of(HttpStatus.SERVICE_UNAVAILABLE))); + } + + @Test + void mapsUnreadableBodiesThroughTheirJsonCause() { + InvalidFormatException invalid = + assertThrows( + InvalidFormatException.class, + () -> new ObjectMapper().readValue("{\"type\":\"VIEW\"}", Body.class)); + assertEquals( + new ApiFailure( + 400, ErrorCodes.INVALID_PARAMETER_VALUE, "type must be one of [TABLE, SCHEMA]"), + ApiExceptionHandler.failureOf(new IllegalArgumentException(invalid))); + } + + @Test + void mapsInvalidParametersByTheirMessage() { + assertEquals( + new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, "maxResults is not a number"), + ApiExceptionHandler.failureOf( + new IllegalArgumentException("maxResults is not a number"))); + } + + @Test + void leavesOtherFailuresToApiFailure() { + assertEquals( + new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "no share"), + ApiExceptionHandler.failureOf( + new CompletionException(ApiException.notFound("no share")))); + assertEquals( + new ApiFailure(500, ErrorCodes.INTERNAL_ERROR, "internal server error"), + ApiExceptionHandler.failureOf(new AssertionError("boom"))); + } + + @Test + void writesTheProtocolErrorBody() throws Exception { + AggregatedHttpResponse response = handle(ApiException.permissionDenied("not the owner")); + + assertEquals(403, response.status().code()); + assertEquals(MediaType.JSON_UTF_8, response.contentType()); + assertNull(response.headers().get(HttpHeaderNames.WWW_AUTHENTICATE)); + JsonNode body = new ObjectMapper().readTree(response.contentUtf8()); + assertEquals(ErrorCodes.PERMISSION_DENIED, body.get("errorCode").asText()); + assertEquals("not the owner", body.get("message").asText()); + } + + @Test + void namesTheBearerSchemeOnUnauthenticated() { + AggregatedHttpResponse response = handle(ApiException.unauthenticated("token is missing")); + + assertEquals(401, response.status().code()); + assertEquals("Bearer", response.headers().get(HttpHeaderNames.WWW_AUTHENTICATE)); + } + + @Test + void decoratesWhatAServiceThrowsAndWhatItsResponseFailsWith() throws Exception { + HttpService throwing = + (ctx, req) -> { + throw ApiException.notFound("thrown"); + }; + HttpService failing = (ctx, req) -> HttpResponse.ofFailure(ApiException.notFound("failed")); + + assertEquals(404, serve(throwing.decorate(handler)).status().code()); + assertEquals(404, serve(failing.decorate(handler)).status().code()); + } + + private AggregatedHttpResponse handle(Throwable cause) { + HttpRequest req = HttpRequest.of(HttpMethod.GET, "/"); + return handler.handleException(ServiceRequestContext.of(req), req, cause).aggregate().join(); + } + + private static AggregatedHttpResponse serve(HttpService service) throws Exception { + HttpRequest req = HttpRequest.of(HttpMethod.GET, "/"); + return service.serve(ServiceRequestContext.of(req), req).aggregate().join(); + } + + record Body(AssetType type) {} +} diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index a3dc2767f..3237580db 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -78,11 +78,15 @@ void mapsOtherCatalogFailuresAsBadGateway() { ErrorCodes.CATALOG_ERROR, "catalog unreachable", new CatalogException("catalog unreachable")); + } + + @Test + void mapsCatalogAuthorizationFailuresAsPermissionDenied() { assertFailure( - 502, - ErrorCodes.CATALOG_ERROR, - "user lacks USE CATALOG", - new CatalogAuthorizationException("user lacks USE CATALOG")); + 403, + ErrorCodes.PERMISSION_DENIED, + "user lacks CREATE_SHARE", + new CatalogAuthorizationException("user lacks CREATE_SHARE")); } @Test diff --git a/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java b/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java index a6a413a9b..2ead58d6b 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java +++ b/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java @@ -1,13 +1,15 @@ package io.opensharing; -import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.server.HttpService; +import com.linecorp.armeria.server.HttpStatusException; import com.linecorp.armeria.server.Server; import io.opensharing.catalog.CatalogConnector; import io.opensharing.catalog.local.LocalCatalogConnector; import io.opensharing.catalog.local.LocalCatalogLoader; import io.opensharing.config.ServerProperties; import io.opensharing.config.ServerProperties.Property; +import io.opensharing.http.ApiExceptionHandler; import io.opensharing.runtime.OpenSharing; import java.util.Locale; @@ -47,11 +49,14 @@ public static void main(String[] args) throws InterruptedException { */ public static OpenSharingServer start(ServerProperties properties) { OpenSharing openSharing = OpenSharing.builder().catalog(catalog(properties)).build(); + HttpService notFound = + (ctx, req) -> { + throw HttpStatusException.of(HttpStatus.NOT_FOUND); + }; Server server = Server.builder() .http(properties.getPort()) - // Armeria refuses to start without a service; routes nothing serves are not found. - .serviceUnder("/", (ctx, req) -> HttpResponse.of(HttpStatus.NOT_FOUND)) + .serviceUnder("/", notFound.decorate(new ApiExceptionHandler())) .build(); try { server.start().join(); diff --git a/server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java b/server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java deleted file mode 100644 index b1b0c052e..000000000 --- a/server/opensharing-server/src/main/java/io/opensharing/http/GlobalExceptionHandler.java +++ /dev/null @@ -1,61 +0,0 @@ -package io.opensharing.http; - -import com.fasterxml.jackson.core.JsonProcessingException; -import org.springframework.http.ResponseEntity; -import org.springframework.http.converter.HttpMessageNotReadableException; -import org.springframework.web.bind.MethodArgumentNotValidException; -import org.springframework.web.bind.MissingServletRequestParameterException; -import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.bind.annotation.RestControllerAdvice; -import org.springframework.web.method.annotation.HandlerMethodValidationException; -import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; -import org.springframework.web.servlet.resource.NoResourceFoundException; - -/** Renders every failure as {@code {errorCode, message}}. */ -@RestControllerAdvice -public class GlobalExceptionHandler { - - @ExceptionHandler(Exception.class) - public ResponseEntity handle(Exception e) { - ApiFailure failure = failureOf(e); - return ResponseEntity.status(failure.status()) - .body(new ErrorResponse(failure.errorCode(), failure.message())); - } - - /** Maps Spring MVC's own failures; everything else is mapped by {@link ApiFailure}. */ - static ApiFailure failureOf(Exception e) { - return switch (e) { - case MethodArgumentNotValidException invalid -> - new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, describe(invalid)); - case HandlerMethodValidationException ignored -> - new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, "request validation failed"); - case MethodArgumentTypeMismatchException mistyped -> - new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, describe(mistyped)); - case MissingServletRequestParameterException missing -> - new ApiFailure( - 400, ErrorCodes.INVALID_PARAMETER_VALUE, missing.getParameterName() + " is required"); - case HttpMessageNotReadableException unreadable -> - unreadable.getCause() instanceof JsonProcessingException json - ? ApiFailure.of(json) - : new ApiFailure(400, ErrorCodes.MALFORMED_REQUEST, "request body is malformed"); - case NoResourceFoundException ignored -> - new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "the endpoint does not exist"); - default -> ApiFailure.of(e); - }; - } - - private static String describe(MethodArgumentNotValidException invalid) { - return invalid.getBindingResult().getFieldErrors().stream() - .findFirst() - .map(error -> error.getField() + " " + error.getDefaultMessage()) - .orElse("request validation failed"); - } - - private static String describe(MethodArgumentTypeMismatchException mistyped) { - Class wanted = mistyped.getRequiredType(); - boolean numeric = wanted == Integer.class || wanted == Long.class; - return numeric - ? mistyped.getName() + " must be a number" - : mistyped.getName() + " is not a valid value"; - } -} diff --git a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java b/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java deleted file mode 100644 index 8b36f1a67..000000000 --- a/server/opensharing-server/src/test/java/io/opensharing/http/GlobalExceptionHandlerTest.java +++ /dev/null @@ -1,124 +0,0 @@ -package io.opensharing.http; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; - -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.exc.InvalidFormatException; -import io.opensharing.catalog.AssetType; -import java.util.List; -import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.context.support.DefaultMessageSourceResolvable; -import org.springframework.core.MethodParameter; -import org.springframework.http.converter.HttpMessageNotReadableException; -import org.springframework.mock.http.MockHttpInputMessage; -import org.springframework.test.web.servlet.MockMvc; -import org.springframework.validation.BeanPropertyBindingResult; -import org.springframework.validation.FieldError; -import org.springframework.validation.method.MethodValidationResult; -import org.springframework.validation.method.ParameterValidationResult; -import org.springframework.web.bind.MethodArgumentNotValidException; -import org.springframework.web.bind.MissingServletRequestParameterException; -import org.springframework.web.method.annotation.HandlerMethodValidationException; -import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; - -@SpringBootTest( - properties = { - "spring.datasource.url=jdbc:h2:mem:http-errors;DB_CLOSE_DELAY=-1", - "opensharing.catalog.type=local", - "opensharing.catalog.local.file=classpath:local-catalog.yml" - }) -@AutoConfigureMockMvc -class GlobalExceptionHandlerTest { - - @Autowired private MockMvc mvc; - - @Test - void answersUnknownRoutesWithTheProtocolErrorBody() throws Exception { - mvc.perform(get("/no-such-route")) - .andExpect(status().isNotFound()) - .andExpect(jsonPath("$.errorCode").value(ErrorCodes.RESOURCE_DOES_NOT_EXIST)) - .andExpect(jsonPath("$.message").value("the endpoint does not exist")); - } - - @Test - void mapsInvalidBodiesWithTheFirstFieldError() throws Exception { - BeanPropertyBindingResult result = new BeanPropertyBindingResult(new Body(null), "body"); - result.addError(new FieldError("body", "type", "must not be null")); - assertFailure( - ErrorCodes.INVALID_PARAMETER_VALUE, - "type must not be null", - new MethodArgumentNotValidException(maxResults(), result)); - } - - @Test - void mapsMethodValidationFailures() throws Exception { - MethodParameter maxResults = maxResults(); - ParameterValidationResult invalid = - new ParameterValidationResult( - maxResults, - 0, - List.of(new DefaultMessageSourceResolvable(null, null, "must be greater than 0"))); - assertFailure( - ErrorCodes.INVALID_PARAMETER_VALUE, - "request validation failed", - new HandlerMethodValidationException( - MethodValidationResult.create(new Object(), maxResults.getMethod(), List.of(invalid)))); - } - - @Test - void mapsMistypedParameters() throws Exception { - assertFailure( - ErrorCodes.INVALID_PARAMETER_VALUE, - "maxResults must be a number", - new MethodArgumentTypeMismatchException( - "ten", Integer.class, "maxResults", maxResults(), null)); - } - - @Test - void mapsMissingParameters() { - assertFailure( - ErrorCodes.INVALID_PARAMETER_VALUE, - "maxResults is required", - new MissingServletRequestParameterException("maxResults", "Integer")); - } - - @Test - void mapsUnreadableBodiesThroughTheirJsonCause() { - InvalidFormatException invalid = - assertThrows( - InvalidFormatException.class, - () -> new ObjectMapper().readValue("{\"type\":\"VIEW\"}", Body.class)); - assertFailure( - ErrorCodes.INVALID_PARAMETER_VALUE, - "type must be one of [TABLE, SCHEMA]", - new HttpMessageNotReadableException( - "unreadable", invalid, new MockHttpInputMessage(new byte[0]))); - } - - @Test - void leavesOtherFailuresToApiFailure() { - ApiFailure failure = GlobalExceptionHandler.failureOf(ApiException.notFound("no share")); - assertEquals(new ApiFailure(404, ErrorCodes.RESOURCE_DOES_NOT_EXIST, "no share"), failure); - } - - private static void assertFailure(String errorCode, String message, Exception e) { - assertEquals(new ApiFailure(400, errorCode, message), GlobalExceptionHandler.failureOf(e)); - } - - private static MethodParameter maxResults() throws NoSuchMethodException { - return new MethodParameter(Endpoint.class.getDeclaredMethod("list", Integer.class), 0); - } - - record Body(AssetType type) {} - - interface Endpoint { - void list(Integer maxResults); - } -} diff --git a/server/opensharing-server/src/test/java/io/opensharing/http/UnknownRouteTest.java b/server/opensharing-server/src/test/java/io/opensharing/http/UnknownRouteTest.java new file mode 100644 index 000000000..d9aae590f --- /dev/null +++ b/server/opensharing-server/src/test/java/io/opensharing/http/UnknownRouteTest.java @@ -0,0 +1,30 @@ +package io.opensharing.http; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import io.opensharing.OpenSharingServer; +import io.opensharing.config.ServerProperties; +import java.util.Properties; +import org.junit.jupiter.api.Test; + +class UnknownRouteTest { + + @Test + void answersUnknownRoutesWithTheProtocolErrorBody() throws Exception { + Properties props = new Properties(); + props.setProperty("server.port", "0"); + try (OpenSharingServer server = OpenSharingServer.start(new ServerProperties(props))) { + WebClient client = WebClient.of("http://127.0.0.1:" + server.port()); + AggregatedHttpResponse response = client.get("/no-such-route").aggregate().join(); + + assertEquals(404, response.status().code()); + JsonNode body = new ObjectMapper().readTree(response.contentUtf8()); + assertEquals(ErrorCodes.RESOURCE_DOES_NOT_EXIST, body.get("errorCode").asText()); + assertEquals("the endpoint does not exist", body.get("message").asText()); + } + } +} From dbfda9f33d572a89e13af1c1e93f94d47a154b46 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Wed, 14 Oct 2026 12:18:27 -0700 Subject: [PATCH 09/10] Expose the server's port for the unknown route test. --- .../src/main/java/io/opensharing/OpenSharingServer.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java b/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java index 2ead58d6b..864f6db0c 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java +++ b/server/opensharing-server/src/main/java/io/opensharing/OpenSharingServer.java @@ -67,6 +67,11 @@ public static OpenSharingServer start(ServerProperties properties) { return new OpenSharingServer(server, openSharing); } + /** The port the server listens on, which it picks itself when configured with 0. */ + public int port() { + return server.activeLocalPort(); + } + /** The OpenSharing instance built from the server's settings. */ public OpenSharing openSharing() { return openSharing; From 9bbdbd8b336e7a7e3c404f1b96a4e3d250011459 Mon Sep 17 00:00:00 2001 From: Sun Cao Date: Wed, 14 Oct 2026 12:18:28 -0700 Subject: [PATCH 10/10] Report a token the catalog cannot authenticate as a 401. --- .../exception/CatalogAuthenticationException.java | 4 ++-- .../exception/CatalogAuthorizationException.java | 4 ++-- .../src/main/java/io/opensharing/http/ApiFailure.java | 9 ++------- .../test/java/io/opensharing/http/ApiFailureTest.java | 10 +++++----- .../catalog/local/LocalCatalogConnector.java | 6 +++--- .../catalog/local/LocalCatalogConnectorTest.java | 4 ++-- 6 files changed, 16 insertions(+), 21 deletions(-) diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthenticationException.java b/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthenticationException.java index e6ab40644..1f8e1c4e1 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthenticationException.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthenticationException.java @@ -1,8 +1,8 @@ package io.opensharing.exception; /** - * The catalog rejected OpenSharing's own credentials, so the server cannot reach the catalog at - * all. A server misconfiguration, not a problem with the end user's request. + * The catalog could not authenticate the caller on {@code CatalogConnector#authorize}: their token + * is missing or invalid. */ public class CatalogAuthenticationException extends CatalogException { diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthorizationException.java b/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthorizationException.java index ffedd0459..a86e6d981 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthorizationException.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/exception/CatalogAuthorizationException.java @@ -1,8 +1,8 @@ package io.opensharing.exception; /** - * The catalog refused the end user on {@code CatalogConnector#authorize}: their token is missing or - * invalid, or they lack the requested privilege. + * The catalog authenticated the caller on {@code CatalogConnector#authorize}, but they lack the + * requested privilege. */ public class CatalogAuthorizationException extends CatalogException { diff --git a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java index da2637f38..4db076005 100644 --- a/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java +++ b/server/opensharing-server-core/src/main/java/io/opensharing/http/ApiFailure.java @@ -32,13 +32,8 @@ public static ApiFailure of(Exception e) { new ApiFailure(403, ErrorCodes.PERMISSION_DENIED, denied.getMessage()); case UnsupportedAssetTypeException unsupported -> new ApiFailure(400, ErrorCodes.INVALID_PARAMETER_VALUE, unsupported.getMessage()); - case CatalogAuthenticationException rejected -> { - log.error("The sharing server could not authenticate to the catalog", rejected); - yield new ApiFailure( - 502, - ErrorCodes.CATALOG_ERROR, - "the sharing server could not authenticate to the catalog"); - } + case CatalogAuthenticationException rejected -> + new ApiFailure(401, ErrorCodes.UNAUTHENTICATED, rejected.getMessage()); case CatalogAuthorizationException refused -> new ApiFailure(403, ErrorCodes.PERMISSION_DENIED, refused.getMessage()); case CatalogException failed -> { diff --git a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java index 3237580db..b33ee9653 100644 --- a/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java +++ b/server/opensharing-server-core/src/test/java/io/opensharing/http/ApiFailureTest.java @@ -63,12 +63,12 @@ void mapsUnsupportedAssetTypesAsBadRequest() { } @Test - void hidesCatalogAuthenticationFailuresBehindBadGateway() { + void mapsCatalogAuthenticationFailuresAsUnauthenticated() { assertFailure( - 502, - ErrorCodes.CATALOG_ERROR, - "the sharing server could not authenticate to the catalog", - new CatalogAuthenticationException("token for svc-sharing expired")); + 401, + ErrorCodes.UNAUTHENTICATED, + "invalid bearer token", + new CatalogAuthenticationException("invalid bearer token")); } @Test diff --git a/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java b/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java index 247b1a95f..ce8360c0f 100644 --- a/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java +++ b/server/opensharing-server/src/main/java/io/opensharing/catalog/local/LocalCatalogConnector.java @@ -10,7 +10,7 @@ import io.opensharing.exception.AssetNotFoundException; import io.opensharing.catalog.AssetType; import io.opensharing.catalog.CatalogConnector; -import io.opensharing.exception.CatalogAuthorizationException; +import io.opensharing.exception.CatalogAuthenticationException; import io.opensharing.exception.CatalogException; import io.opensharing.catalog.CloudProvider; import io.opensharing.catalog.CredentialRequest; @@ -203,7 +203,7 @@ public UserContext authorize(AuthContext auth, Privilege privilege) { } String token = auth == null || auth.user() == null ? null : auth.user().userAuthToken(); if (token == null || token.isBlank()) { - throw new CatalogAuthorizationException("missing bearer token"); + throw new CatalogAuthenticationException("missing bearer token"); } return principals.stream() .filter(principal -> token.equals(principal.bearerToken())) @@ -211,7 +211,7 @@ public UserContext authorize(AuthContext auth, Privilege privilege) { .map( principal -> new UserContext(principal.userId(), principal.bearerToken(), principal.userName())) - .orElseThrow(() -> new CatalogAuthorizationException("invalid bearer token")); + .orElseThrow(() -> new CatalogAuthenticationException("invalid bearer token")); } private static boolean covers(LocalCatalogFile.Asset asset, String location) { diff --git a/server/opensharing-server/src/test/java/io/opensharing/catalog/local/LocalCatalogConnectorTest.java b/server/opensharing-server/src/test/java/io/opensharing/catalog/local/LocalCatalogConnectorTest.java index 7abff0368..717bcfbbd 100644 --- a/server/opensharing-server/src/test/java/io/opensharing/catalog/local/LocalCatalogConnectorTest.java +++ b/server/opensharing-server/src/test/java/io/opensharing/catalog/local/LocalCatalogConnectorTest.java @@ -13,7 +13,7 @@ import io.opensharing.catalog.AssetPage; import io.opensharing.exception.AssetNotFoundException; import io.opensharing.catalog.AssetType; -import io.opensharing.exception.CatalogAuthorizationException; +import io.opensharing.exception.CatalogAuthenticationException; import io.opensharing.exception.CatalogException; import io.opensharing.catalog.CloudProvider; import io.opensharing.catalog.CredentialRequest; @@ -423,7 +423,7 @@ void authorizesConfiguredLocalPrincipals() { assertEquals("catalog-alice-id", alice.userId()); assertEquals("alice", alice.userName()); assertThrows( - CatalogAuthorizationException.class, + CatalogAuthenticationException.class, () -> connector.authorize( new AuthContext(null, new UserContext(null, "mallory-token", null)), null));