fix(rest): align error handlers with Java implementation#763
Conversation
| IsError(ErrorKind::kNoSuchPlanTask)); | ||
| } | ||
|
|
||
| TEST(ErrorHandlersTest, OAuthErrorHandlerMapsInvalidClientToNotAuthorized) { |
There was a problem hiding this comment.
Can we add one test that goes through the OAuth error parsing path, not just OAuthErrorHandler::Accept with a prebuilt ErrorResponse? Otherwise this won't catch regressions in the new TryParseOAuthErrorResponse logic or accidentally using the default handler for token requests.
There was a problem hiding this comment.
Sounds good. I'll add a test to cover this path.
There was a problem hiding this comment.
Added a test that exercises OAuthErrorHandler::ParseResponse with an OAuth error body, then verifies the parsed response maps correctly through Accept.
huan233usc
left a comment
There was a problem hiding this comment.
Mostly LGTM, thank you!
| } | ||
| auto parse_result = TryParseErrorResponse(response.text); | ||
| auto parse_result = | ||
| dynamic_cast<const OAuthErrorHandler*>(&error_handler) != nullptr |
There was a problem hiding this comment.
This looks a bit ugly. Maybe we should add a parseResponse interface to ErrorHandler, similar to the Java impl.
@wgtmac WDYT?
While reviewing apache#614, I noticed that `PlanErrorHandler::Accept` does not match the behavior of the Java implementation. A detailed comparison with Java's `ErrorHandlers` revealed several gaps between iceberg-cpp and Iceberg Java. Some of these are oversights in the original implementation, while others correspond to improvements that were made later in Iceberg Java, including: * apache/iceberg#13143 * apache/iceberg#14927 * apache/iceberg#15051 * apache/iceberg#16059 This PR closes those gaps and brings the error handling behavior in iceberg-cpp closer to the Java implementation.
78e88d1 to
29a2a72
Compare
|
Thanks @zhjwpku for working on this and @huan233usc for the review! |
While reviewing #614, I noticed that
PlanErrorHandler::Acceptdoes not match the behavior of the Java implementation.A detailed comparison with Java's
ErrorHandlersrevealed several gaps between iceberg-cpp and Iceberg Java. Some of these are oversights in the original implementation, while others correspond to improvements that were made later in Iceberg Java, including:This PR closes those gaps and brings the error handling behavior in iceberg-cpp closer to the Java implementation.