diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java b/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java index 67ce6ced18f0..c1fd9297362c 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java @@ -42,7 +42,10 @@ public static ErrorHandler getInstance() { @Override public void accept(ErrorResponse error, String requestId) { - int code = error.getCode(); + Integer errorCode = error.getCode(); + // HttpClient always resolves the code before calling this, but the response may also be + // deserialized directly, and then "code" is absent whenever the server omits it. + int code = errorCode == null ? 0 : errorCode; String message; if (DEFAULT_REQUEST_ID.equals(requestId)) { message = error.getMessage(); diff --git a/paimon-api/src/main/java/org/apache/paimon/rest/responses/ErrorResponse.java b/paimon-api/src/main/java/org/apache/paimon/rest/responses/ErrorResponse.java index bfc9e3bf4e70..899b0896afc9 100644 --- a/paimon-api/src/main/java/org/apache/paimon/rest/responses/ErrorResponse.java +++ b/paimon-api/src/main/java/org/apache/paimon/rest/responses/ErrorResponse.java @@ -73,6 +73,7 @@ public class ErrorResponse implements RESTResponse { @JsonProperty(FIELD_MESSAGE) private final String message; + @Nullable @JsonProperty(FIELD_CODE) private final Integer code; @@ -81,13 +82,18 @@ public ErrorResponse( @Nullable @JsonProperty(FIELD_RESOURCE_TYPE) String resourceType, @Nullable @JsonProperty(FIELD_RESOURCE_NAME) String resourceName, @JsonProperty(FIELD_MESSAGE) String message, - @JsonProperty(FIELD_CODE) int code) { + @Nullable @JsonProperty(FIELD_CODE) Integer code) { this.resourceType = resourceType; this.resourceName = resourceName; this.message = message; this.code = code; } + /** Retained for callers compiled against the primitive {@code code} descriptor. */ + public ErrorResponse(String resourceType, String resourceName, String message, int code) { + this(resourceType, resourceName, message, (Integer) code); + } + @JsonGetter(FIELD_MESSAGE) public String getMessage() { return message; @@ -103,6 +109,7 @@ public String getResourceName() { return resourceName; } + @Nullable @JsonGetter(FIELD_CODE) public Integer getCode() { return code; diff --git a/paimon-api/src/test/java/org/apache/paimon/rest/responses/ErrorResponseTest.java b/paimon-api/src/test/java/org/apache/paimon/rest/responses/ErrorResponseTest.java new file mode 100644 index 000000000000..7b977a0a3226 --- /dev/null +++ b/paimon-api/src/test/java/org/apache/paimon/rest/responses/ErrorResponseTest.java @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.rest.responses; + +import org.apache.paimon.rest.RESTApi; + +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonCreator; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Constructor; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; + +/** Tests for {@link ErrorResponse}. */ +public class ErrorResponseTest { + + private static final Class[] PRIMITIVE_CODE_CTOR = { + String.class, String.class, String.class, int.class + }; + + @Test + public void testPrimitiveCodeConstructorDescriptorIsRetained() { + // The descriptor REST server implementations compiled against an earlier paimon-api + // invoke. A source level new ErrorResponse(a, b, c, 404) would still compile if it were + // deleted, because javac boxes into the Integer overload, so assert it reflectively. + assertThatCode(() -> ErrorResponse.class.getConstructor(PRIMITIVE_CODE_CTOR)) + .doesNotThrowAnyException(); + } + + @Test + public void testExactlyOneJsonCreatorAndItAcceptsNullableCode() throws Exception { + List> creators = + Arrays.stream(ErrorResponse.class.getDeclaredConstructors()) + .filter(c -> c.isAnnotationPresent(JsonCreator.class)) + .collect(Collectors.toList()); + + assertThat(creators).hasSize(1); + assertThat(creators.get(0).getParameterTypes()) + .containsExactly(String.class, String.class, String.class, Integer.class); + // the primitive overload must stay invisible to Jackson, otherwise an absent code + // deserializes to 0 again + assertThat( + ErrorResponse.class + .getConstructor(PRIMITIVE_CODE_CTOR) + .isAnnotationPresent(JsonCreator.class)) + .isFalse(); + } + + @Test + public void testCodeIsAbsentOnTheWireRatherThanZero() throws Exception { + assertThat(RESTApi.fromJson("{\"message\":\"x\"}", ErrorResponse.class).getCode()).isNull(); + assertThat( + RESTApi.fromJson("{\"message\":\"x\",\"code\":null}", ErrorResponse.class) + .getCode()) + .isNull(); + assertThat( + RESTApi.fromJson("{\"message\":\"x\",\"code\":404}", ErrorResponse.class) + .getCode()) + .isEqualTo(404); + } + + @Test + public void testBothConstructorsAgree() throws Exception { + assertThat(new ErrorResponse("TABLE", "t", "m", 404).getCode()).isEqualTo(404); + assertThat(new ErrorResponse("TABLE", "t", "m", (Integer) null).getCode()).isNull(); + assertThat(RESTApi.toJson(new ErrorResponse(null, null, "m", (Integer) null))) + .contains("\"code\":null"); + } +} diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java index 9f8c6904664f..3cdfe7c938c7 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java @@ -103,6 +103,20 @@ public void testErrorMessageIsNotReadAsAFormatString() { } } + @Test + public void testNullCodeDoesNotNpeAndFallsThrough() { + // the code is optional in the error schema, so an omitted one reaches the handler as + // null and must not unbox + RESTException exception = + assertThrows( + RESTException.class, + () -> + defaultErrorHandler.accept( + new ErrorResponse(null, null, "message", (Integer) null), + DEFAULT_REQUEST_ID)); + assertTrue(exception.getMessage().contains("message")); + } + private ErrorResponse generateErrorResponse(int code) { return new ErrorResponse(null, null, "message", code); } diff --git a/paimon-core/src/test/java/org/apache/paimon/rest/HttpClientTest.java b/paimon-core/src/test/java/org/apache/paimon/rest/HttpClientTest.java index 5bdc553fdf6a..9aab6e504360 100644 --- a/paimon-core/src/test/java/org/apache/paimon/rest/HttpClientTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/rest/HttpClientTest.java @@ -23,6 +23,8 @@ import org.apache.paimon.rest.auth.RESTAuthFunction; import org.apache.paimon.rest.auth.RESTAuthParameter; import org.apache.paimon.rest.exceptions.BadRequestException; +import org.apache.paimon.rest.exceptions.ForbiddenException; +import org.apache.paimon.rest.exceptions.NoSuchResourceException; import org.apache.paimon.rest.exceptions.RESTException; import org.apache.paimon.rest.responses.ErrorResponse; @@ -45,6 +47,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; /** Test for {@link HttpClient}. */ @@ -255,6 +258,23 @@ public void testUrl() { assertEquals(restAuthParameter.parameters().get(queryKey), queryParameters.get(queryKey)); } + @Test + public void testErrorCodeFallsBackToHttpStatus() throws Exception { + // "code" is optional in the error schema, so an error body may omit it. The HTTP status + // has to be used then, otherwise a 404 no longer maps to NoSuchResourceException. + assertNull(RESTApi.fromJson("{\"message\":\"x\"}", ErrorResponse.class).getCode()); + server.enqueueResponse("{\"message\":\"Table t does not exist\"}", 404); + assertThrows( + NoSuchResourceException.class, + () -> httpClient.get(MOCK_PATH, MockRESTData.class, restAuthFunction)); + + // classification follows the status, so a different one maps differently + server.enqueueResponse("{\"message\":\"denied\"}", 403); + assertThrows( + ForbiddenException.class, + () -> httpClient.get(MOCK_PATH, MockRESTData.class, restAuthFunction)); + } + private Map getParameters(String path) { String[] paths = path.split("\\?"); if (paths.length == 1) { @@ -293,6 +313,10 @@ public void testGetWithUnparsableJsonErrorResponse() { Assertions.assertTrue( e.getMessage().contains("Empty error message"), "Parsed-but-empty message must not be labelled unparseable"); + Assertions.assertTrue( + e.getMessage().contains("403"), + "The HTTP status must be reported, not the absent body code: " + + e.getMessage()); } }