Skip to content

Navigation Menu

Sign in
Sign up

[api] Fall back to the HTTP status when the error body omits code - #9661

Merged
JingsongLi merged 1 commit into
apache:master from
jackylee-ch:rest-error-code-nullable
Sep 12, 2026
Merged

[api] Fall back to the HTTP status when the error body omits code #9661
JingsongLi merged 1 commit into
apache:master from
jackylee-ch:rest-error-code-nullable

Conversation

@jackylee-ch

@jackylee-ch jackylee-ch commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Purpose

ErrorResponse's field and getter are Integer, but the @JsonCreator took a primitive int. An absent creator parameter defaults to 0, and the shared mapper leaves FAIL_ON_NULL_FOR_PRIMITIVES off, so an explicit "code": null does too. code has no required: entry in rest-catalog-open-api.yaml, so a server may legally omit it, and the HTTP-status fallback HttpClient has carried since #8721 was then unreachable.

That shows up two ways: a 404 with a message was classified as RESTException instead of NoSuchResourceException, and an empty or unparseable body reported (HTTP 0).

The Integer constructor stays the @JsonCreator; an unannotated int overload delegates to it, retaining the (String,String,String,int) descriptor — master carried that pair between 286212af7 and ee08d8328. getCode() is @Nullable.

Tests

ErrorResponseTest (new), HttpClientTest, DefaultErrorHandlerTest. Reverting the production files:

testCodeIsAbsentOnTheWireRatherThanZero expected: null but was: 0
testErrorCodeFallsBackToHttpStatus expected: <null> but was: <0>
testGetWithUnparsableJsonErrorResponse Empty error message (HTTP 0)

Written with Claude Code; verification is mine.

jackylee-ch force-pushed the rest-error-code-nullable branch 2 times, most recently from e0dc764 to 24a1767 Compare September 7, 2026 11:14

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the HTTP status when the body omits code fixes a real REST error-classification problem. The missing-code regression passes after recompiling the head response and HttpClient. There is also an avoidable binary-compatibility break in the public response constructor; details are inline.

An old compiled HttpClient combined with the new response class fails with NoSuchMethodError instead of NoSuchResourceException on the same 404. This is a compatibility probe, not a failure of a fully rebuilt checkout.

@Nullable @JsonProperty(FIELD_RESOURCE_NAME) String resourceName,
@JsonProperty(FIELD_MESSAGE) String message,
@JsonProperty(FIELD_CODE) int code) {
@Nullable @JsonProperty(FIELD_CODE) Integer code) {

@JingsongLi JingsongLi Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Retain the existing primitive constructor for compiled API consumers

Changing the public constructor's final parameter from int to Integer removes the JVM descriptor (String,String,String,int). Autoboxing only helps newly compiled source: existing REST clients/extensions compiled against paimon-api still invoke the removed method. I reproduced NoSuchMethodError at HttpClient.buildErrorResponse on a missing-code 404 when loading the changed ErrorResponse with an already compiled caller; rebuilding that caller makes the functional regression pass.

Keep the nullable Integer constructor as the JsonCreator and retain an unannotated int overload delegating to it. That preserves the existing binary entry point without losing the distinction between an omitted code and HTTP 0. Please include a small precompiled-caller compatibility check.

Copy link
Copy Markdown
Contributor Author

The overload is in, unannotated with bare params, and RequestJacksonCompatibilityTest — the creator-scanning guard — still passes with both constructors present, so resolution is settled empirically. I did not build a precompiled-caller loader: a checked-in .class fixture is not reviewable, so the check is a reflective getConstructor(..,int.class) plus a single-@JsonCreator assertion in a new paimon-api test — RESTApiJsonTest is claimed by #9640. One correction: HttpClient and ErrorResponse ship in the same jar, so this PR changes no HttpClient 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 apache#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.

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing-code HTTP error classification bug has a concrete REST catalog impact, and the revised constructor now addresses my prior binary-compatibility finding. The primitive (String,String,String,int) overload is preserved without a JsonCreator, while the single nullable Integer creator lets missing/null JSON code reach the existing HTTP-status fallback. I compiled a caller against the base primitive constructor and ran the unchanged caller against this head successfully. All 28 focused ErrorResponse/DefaultErrorHandler/HttpClient tests passed, including 403/404 bodies without code and direct null-code handling, with the affected classes and exact-head HttpClient compiled on JDK 8 against local cached dependencies. No remaining actionable finding in this change; this is targeted validation, not a full Maven/CI pass.

JingsongLi merged commit ff8670e into apache:master Sep 12, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JingsongLi JingsongLi JingsongLi left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /