Skip to content

Commit 1af7441

Browse files
committed
[api] Fall back to the HTTP status when the error body omits code
ErrorResponse's field and getter are Integer, but the JsonCreator took a primitive int and the shared mapper leaves FAIL_ON_NULL_FOR_PRIMITIVES off. "code" has no required: entry in rest-catalog-open-api.yaml, so a server may legally omit it; it then deserialized to 0, and the HTTP-status fallback that HttpClient has carried since #8721 was unreachable for every instance this codebase can build. A 404 surfaced as RESTException reading (HTTP 0) instead of NoSuchResourceException. DefaultErrorHandler now tolerates a null code rather than unboxing it. The nullable Integer constructor stays the JsonCreator; an unannotated int overload delegates to it, so the (String,String,String,int) descriptor that out-of-tree callers were compiled against is retained. Master carried that same constructor pair between 286212a and ee08d83.
1 parent 2f671d6 commit 1af7441

5 files changed

Lines changed: 140 additions & 2 deletions

File tree

‎paimon-api/src/main/java/org/apache/paimon/rest/DefaultErrorHandler.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,10 @@ public static ErrorHandler getInstance() {
4242

4343
@Override
4444
public void accept(ErrorResponse error, String requestId) {
45-
int code = error.getCode();
45+
Integer errorCode = error.getCode();
46+
// HttpClient always resolves the code before calling this, but the response may also be
47+
// deserialized directly, and then "code" is absent whenever the server omits it.
48+
int code = errorCode == null ? 0 : errorCode;
4649
String message;
4750
if (DEFAULT_REQUEST_ID.equals(requestId)) {
4851
message = error.getMessage();

‎paimon-api/src/main/java/org/apache/paimon/rest/responses/ErrorResponse.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ public class ErrorResponse implements RESTResponse {
7373
@JsonProperty(FIELD_MESSAGE)
7474
private final String message;
7575

76+
@Nullable
7677
@JsonProperty(FIELD_CODE)
7778
private final Integer code;
7879

@@ -81,13 +82,18 @@ public ErrorResponse(
8182
@Nullable @JsonProperty(FIELD_RESOURCE_TYPE) String resourceType,
8283
@Nullable @JsonProperty(FIELD_RESOURCE_NAME) String resourceName,
8384
@JsonProperty(FIELD_MESSAGE) String message,
84-
@JsonProperty(FIELD_CODE) int code) {
85+
@Nullable @JsonProperty(FIELD_CODE) Integer code) {
8586
this.resourceType = resourceType;
8687
this.resourceName = resourceName;
8788
this.message = message;
8889
this.code = code;
8990
}
9091

92+
/** Retained for callers compiled against the primitive {@code code} descriptor. */
93+
public ErrorResponse(String resourceType, String resourceName, String message, int code) {
94+
this(resourceType, resourceName, message, (Integer) code);
95+
}
96+
9197
@JsonGetter(FIELD_MESSAGE)
9298
public String getMessage() {
9399
return message;
@@ -103,6 +109,7 @@ public String getResourceName() {
103109
return resourceName;
104110
}
105111

112+
@Nullable
106113
@JsonGetter(FIELD_CODE)
107114
public Integer getCode() {
108115
return code;
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
package org.apache.paimon.rest.responses;
20+
21+
import org.apache.paimon.rest.RESTApi;
22+
23+
import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.annotation.JsonCreator;
24+
25+
import org.junit.jupiter.api.Test;
26+
27+
import java.lang.reflect.Constructor;
28+
import java.util.Arrays;
29+
import java.util.List;
30+
import java.util.stream.Collectors;
31+
32+
import static org.assertj.core.api.Assertions.assertThat;
33+
import static org.assertj.core.api.Assertions.assertThatCode;
34+
35+
/** Tests for {@link ErrorResponse}. */
36+
public class ErrorResponseTest {
37+
38+
private static final Class<?>[] PRIMITIVE_CODE_CTOR = {
39+
String.class, String.class, String.class, int.class
40+
};
41+
42+
@Test
43+
public void testPrimitiveCodeConstructorDescriptorIsRetained() {
44+
// The descriptor REST server implementations compiled against an earlier paimon-api
45+
// invoke. A source level new ErrorResponse(a, b, c, 404) would still compile if it were
46+
// deleted, because javac boxes into the Integer overload, so assert it reflectively.
47+
assertThatCode(() -> ErrorResponse.class.getConstructor(PRIMITIVE_CODE_CTOR))
48+
.doesNotThrowAnyException();
49+
}
50+
51+
@Test
52+
public void testExactlyOneJsonCreatorAndItAcceptsNullableCode() throws Exception {
53+
List<Constructor<?>> creators =
54+
Arrays.stream(ErrorResponse.class.getDeclaredConstructors())
55+
.filter(c -> c.isAnnotationPresent(JsonCreator.class))
56+
.collect(Collectors.toList());
57+
58+
assertThat(creators).hasSize(1);
59+
assertThat(creators.get(0).getParameterTypes())
60+
.containsExactly(String.class, String.class, String.class, Integer.class);
61+
// the primitive overload must stay invisible to Jackson, otherwise an absent code
62+
// deserializes to 0 again
63+
assertThat(
64+
ErrorResponse.class
65+
.getConstructor(PRIMITIVE_CODE_CTOR)
66+
.isAnnotationPresent(JsonCreator.class))
67+
.isFalse();
68+
}
69+
70+
@Test
71+
public void testCodeIsAbsentOnTheWireRatherThanZero() throws Exception {
72+
assertThat(RESTApi.fromJson("{\"message\":\"x\"}", ErrorResponse.class).getCode()).isNull();
73+
assertThat(
74+
RESTApi.fromJson("{\"message\":\"x\",\"code\":null}", ErrorResponse.class)
75+
.getCode())
76+
.isNull();
77+
assertThat(
78+
RESTApi.fromJson("{\"message\":\"x\",\"code\":404}", ErrorResponse.class)
79+
.getCode())
80+
.isEqualTo(404);
81+
}
82+
83+
@Test
84+
public void testBothConstructorsAgree() throws Exception {
85+
assertThat(new ErrorResponse("TABLE", "t", "m", 404).getCode()).isEqualTo(404);
86+
assertThat(new ErrorResponse("TABLE", "t", "m", (Integer) null).getCode()).isNull();
87+
assertThat(RESTApi.toJson(new ErrorResponse(null, null, "m", (Integer) null)))
88+
.contains("\"code\":null");
89+
}
90+
}

‎paimon-core/src/test/java/org/apache/paimon/rest/DefaultErrorHandlerTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,20 @@ public void testErrorMessageIsNotReadAsAFormatString() {
103103
}
104104
}
105105

106+
@Test
107+
public void testNullCodeDoesNotNpeAndFallsThrough() {
108+
// the code is optional in the error schema, so an omitted one reaches the handler as
109+
// null and must not unbox
110+
RESTException exception =
111+
assertThrows(
112+
RESTException.class,
113+
() ->
114+
defaultErrorHandler.accept(
115+
new ErrorResponse(null, null, "message", (Integer) null),
116+
DEFAULT_REQUEST_ID));
117+
assertTrue(exception.getMessage().contains("message"));
118+
}
119+
106120
private ErrorResponse generateErrorResponse(int code) {
107121
return new ErrorResponse(null, null, "message", code);
108122
}

‎paimon-core/src/test/java/org/apache/paimon/rest/HttpClientTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import org.apache.paimon.rest.auth.RESTAuthFunction;
2424
import org.apache.paimon.rest.auth.RESTAuthParameter;
2525
import org.apache.paimon.rest.exceptions.BadRequestException;
26+
import org.apache.paimon.rest.exceptions.ForbiddenException;
27+
import org.apache.paimon.rest.exceptions.NoSuchResourceException;
2628
import org.apache.paimon.rest.exceptions.RESTException;
2729
import org.apache.paimon.rest.responses.ErrorResponse;
2830

@@ -45,6 +47,7 @@
4547
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
4648
import static org.junit.jupiter.api.Assertions.assertEquals;
4749
import static org.junit.jupiter.api.Assertions.assertFalse;
50+
import static org.junit.jupiter.api.Assertions.assertNull;
4851
import static org.junit.jupiter.api.Assertions.assertThrows;
4952

5053
/** Test for {@link HttpClient}. */
@@ -255,6 +258,23 @@ public void testUrl() {
255258
assertEquals(restAuthParameter.parameters().get(queryKey), queryParameters.get(queryKey));
256259
}
257260

261+
@Test
262+
public void testErrorCodeFallsBackToHttpStatus() throws Exception {
263+
// "code" is optional in the error schema, so an error body may omit it. The HTTP status
264+
// has to be used then, otherwise a 404 no longer maps to NoSuchResourceException.
265+
assertNull(RESTApi.fromJson("{\"message\":\"x\"}", ErrorResponse.class).getCode());
266+
server.enqueueResponse("{\"message\":\"Table t does not exist\"}", 404);
267+
assertThrows(
268+
NoSuchResourceException.class,
269+
() -> httpClient.get(MOCK_PATH, MockRESTData.class, restAuthFunction));
270+
271+
// classification follows the status, so a different one maps differently
272+
server.enqueueResponse("{\"message\":\"denied\"}", 403);
273+
assertThrows(
274+
ForbiddenException.class,
275+
() -> httpClient.get(MOCK_PATH, MockRESTData.class, restAuthFunction));
276+
}
277+
258278
private Map<String, String> getParameters(String path) {
259279
String[] paths = path.split("\\?");
260280
if (paths.length == 1) {
@@ -293,6 +313,10 @@ public void testGetWithUnparsableJsonErrorResponse() {
293313
Assertions.assertTrue(
294314
e.getMessage().contains("Empty error message"),
295315
"Parsed-but-empty message must not be labelled unparseable");
316+
Assertions.assertTrue(
317+
e.getMessage().contains("403"),
318+
"The HTTP status must be reported, not the absent body code: "
319+
+ e.getMessage());
296320
}
297321
}
298322

0 commit comments

Comments
 (0)